-
Notifications
You must be signed in to change notification settings - Fork 21
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.
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
Grid sticky functionality fixed in full pages #2960
Grid sticky functionality fixed in full pages #2960
Conversation
✅ Deploy Preview for ons-design-system-preview ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Other than these two minor content things, looks good
src/components/table-of-contents/example-table-of-contents-sticky-full-page.njk
Outdated
Show resolved
Hide resolved
src/components/table-of-contents/example-table-of-contents-sticky-full-page.njk
Show resolved
Hide resolved
Actually maybe we should add a test to this. One that loads the page example, scrolls to the bottom and checks that the table of contents is still in view. Will stop the bug breaking again. |
Okay sure, I will work on that test and add it in |
All look sgood to me :) One note for next time , I have noticed that your branch doesn't follow the convention stated in the docs here. You can leave this branch names as it is but let's make sure to all stick to the rule defined in the docs :) |
Other than my comments still needing to be addressed and the tests failing this now looks good |
What is the context of this PR?
Fixes: #2835
Previously, the ons grid had the functionality to make columns sticky i.e. have the object have fixed positioning on the screen. This worked when the grid was not contained within any page however was broken when the class was applied to an object within a full page such as the base page template. The change fixes this to work within the full page and adds an additional visual regression test for testing the sticky class in full pages
Additional documentation about this class is also needed and the new VR example can be used for demonstration see ticket: #2969
How to review this PR
Describe the steps required to test the changes (include screenshots if appropriate).
Navigate to the new regression test within the table component and check that the contents section on the page remains in place whilst scrolling.
Checklist
This needs to be completed by the person raising the PR.