Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Direct link to models #5993

Closed
wants to merge 2 commits into from
Closed

Direct link to models #5993

wants to merge 2 commits into from

Conversation

mdhtr
Copy link

@mdhtr mdhtr commented May 8, 2020 •
edited
Loading

Motivation and Context

This PR would cover a part of the feature request #1369 (Deep linking for models).

The part it aims to cover is the ability to directly link to models in the URL.

The goal with this feature would be the following:

  • given that DeepLinking is turned on,
  • when the user specifies a model in the URL,
  • then the UI should open that model and scroll to it

Description

(This PR seems to be the newer version of #5237)

Extend the functionality of the deep-linking plugin:

  • The models section is expanded and scrolled to with #/models
  • The specific model is expanded and scrolled to with #/models/ModelName

Necessary refactors to make the above possible:

  • extract new component for model-container
  • change how the styling is done for the models section

Bugs fixed:

  • the dev-e2e-cypress script was calling the wrong script to start the cypress server
  • the spacing between the first and second item in the models section was different (bigger) than the spacing between the rest of the items.

Known bugs:

  • when a model is open, collapsing and expanding the models section will put the last open model in the path instead of just the models path without the specific model

Help needed!

It would be nice if someone with actual knowledge about ReactJs and SwaggerUi and DeepLinking could help out :)

Is this PR introducing breaking changes?

Please help me to decide if this PR is introducing breaking changes or not!

I think these might be considered as breaking changes:

  • wrapped the models section and it's individual models in wrapper components, so the DOM structure changed.
  • extracted a new React component inbetween Models.jsx and ModelCollapse.jsx. The new component's name is ModelContainer.jsx. This theoretically did not change the DOM structure.
  • modified the styling in _models.scss to accomodate the wrapped component, so the styling mechanism is changed.

[DONE] How to fix the scrolling to the specific model?

The problem is that the zenscroll library cannot get the position of the ModelCollapse component right, so it does not scroll to the right place. I couldn't figure out how to fix this yet, any help is appreciated.

One random idea I had is to somehow include the div with the model-${name} id in the ModelCollapse component, because that element's position seems to be working well for positioning (offsetTop gives back an appropriate number), but this seems to be something that might become a breaking change, so I'm somewhat reluctant to go this way.

UPDATE: I implemented this idea in the second commit and it made the scrolling possible.

How Has This Been Tested?

Manually & added e2e tests.

Checklist

My PR contains...

  • No code changes (src/ is unmodified: changes to documentation, CI, metadata, etc.)
  • Dependency changes (any modification to dependencies in package.json)
  • Bug fixes (non-breaking change which fixes an issue)
  • Improvements (misc. changes to existing features)
  • Features (non-breaking change which adds functionality)

My changes...

  • are breaking changes to a public API (config options, System API, major UI change, etc).
  • are breaking changes to a private API (Redux, component props, utility functions, etc.).
  • are breaking changes to a developer API (npm script behavior changes, new dev system dependencies, etc).
  • are not breaking changes.

Documentation

  • My changes do not require a change to the project documentation.
  • My changes require a change to the project documentation.
  • If yes to above: I have updated the documentation accordingly.

Automated tests

  • My changes can not or do not need to be tested.
  • My changes can and should be tested by unit and/or integration tests.
  • If yes to above: I have added tests to cover my changes.
  • If yes to above: I have taken care to cover edge cases in my tests.
  • All new and existing tests passed.

@mdhtr
Expand models based on path
a547c22 
to expand a model, the path should be #/models/ModelName
to expand the models section, the path should be #/models
scrolling at this point only works for #/models section but not individual models
@mdhtr mdhtr marked this pull request as draft May 9, 2020 07:12
@mdhtr mdhtr changed the title [Work In Progress] Direct link to models Direct link to models May 9, 2020
@mdhtr
Scroll to specific model
c80fa61 
to make scrolling to specific model work, I had to do the following:
- extract a new component from models.jsx for the individual models, one that contains the div with the id as well, so it can be wrapped by the deep-linking plugin. This is model-container.jsx (after the class name on the main element).
- this first step somehow automatically enabled the placement of the correct url into the path when the model is opened and removing that when it is collapsed.
- the styling of the individual models was dependent on the "model-container" class items being on the same level, which was no longer true because of the wrapper. I fixed this by doing the followings:
  - in the e2e tests, I replaced the selectors with id selectors instead of positional selectors.
  - in the _models.scss, I figured that the goal of formatting was that if the models is open, there should be a 20px space around the list of models, while between the models there should be a 15px space.
    - I removed the formatting that was dependent on the model-container classes being on the same level, so the first-of-type and last-of-type and the bottom padding of the section.models.is-open
    - Instead of the removed formatting, I added a top margin for the first child and a bottom margin for the last child of the div that contains the individual models.
  - In the deep-linking wrapper I used a div instead of a span so that the margin formatting is applied properly.

testing:
- adjust test to new component: model-container
- add new cypress e2e tests for deep linking model (I needed to add two data attributes next to the .model-container class to make the element specific enough for the tests)

todo:
- add anchor link to the models with the DeepLink component, and enable testing for this.
@mdhtr mdhtr force-pushed the mdhtr_deep_link_models branch from f1d8db0 to c80fa61 Compare May 10, 2020 18:33
@tim-lai
Copy link
Contributor

tim-lai commented Jun 15, 2020

@mdhtr Thank you for your work in this PR. The separate referenced #5237 was just merged. Please feel free to reopen if this PR is still needed, or create new if #5237 needs any additional improvements. Thanks.

@tim-lai tim-lai closed this Jun 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@mdhtr @tim-lai