-
Notifications
You must be signed in to change notification settings - Fork 66
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
feat(page-grid): upgraded component #2011
Conversation
Any breaking changes that should be called out? |
There are major changes, but I'm not sure any of them would be breaking. That said, it's a good idea to call those out anyway. I'll add those to the PR and we should include them in the release notes as well. |
Layout of those Featured Posts elements looks off in Firefox: Also seeing a horizontal scroll bar being triggered on main nav region. I think we can leave as is for now, as it's outside the scope of page grid itself. But at some point it would be good to come back and address this, or we simply document it and say we leave those things up to the page developers. |
Oh, good catch! Let me take a look. |
@ianmcburnie , I fixed the issues. I left the menu as is but added a general message at the top for developers. If you'd like to rephrase it, let me know. |
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.
LGTM overall. I'm fine approving but one comment about leftover files
dist/carousel/carousel.css
Outdated
-webkit-scroll-snap-type: mandatory; | ||
-webkit-scroll-snap-type: x mandatory; | ||
-ms-scroll-snap-type: mandatory; | ||
-ms-scroll-snap-type: x mandatory; | ||
scroll-snap-type: proximity; | ||
-webkit-scroll-snap-type: x proximity; | ||
-ms-scroll-snap-type: x proximity; | ||
scroll-snap-type: x proximity; | ||
scroll-snap-type: x proximity; |
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.
Hmm this shouldn't be changing. I've seen this section removed again (Luke added it back in one of my PRs). I think you might need to rebuild with new node modules.
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.
Hmm this shouldn't be changing. I've seen this section removed again (Luke added it back in one of my PRs). I think you might need to rebuild with new node modules.
I rebased from 16.0.0
, cleaned and did an npm i
and ...I don't see any diffs. Is it possible support for the property got good enough that the vendor prefixes are no longer needed? I see those vendor prefixes exist in the .less
file also. Maybe the autoprefixer is removing them intentionally?
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.
So this happened to me as well. But when my PR for icons was up, those were reverted.
8edf506
So we need to pick a way and stick with it and make sure it doesn't get reverted.
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.
Maybe your PR for icons branch did not have the latest node-modules
? idk. I'm fine either way. If there is a way to revert it without any manual intervention, I'm all ears. For now, I've tried numerous "system" things to get it to the state you want, and nothing's worked.
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.
This typically happens if on different versions of less-plugin-autoprefix
…thub.com/eBay/skin into 1901-page-grid-layout-gridmargin-updates
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.
Not sure what is happening here, but it seems like something is out of sync. I see a lot of icon changes here.
Let me rebase again. Something wonky is definitely going on. |
…out-gridmargin-updates
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.
Nice work @ArtBlue !
Fixes #1901
Description
css
properties.Notes
I did not move to L4 of media queries since it's still in tech preview version in Safari, but that is one of the last major browsers still outstanding for full support.
Major Changes (some potentially breaking)
page-grid
(inner element) topage-grid-container
. This makespage-grid
responsibly solely for the grid whilepage-grid-container
handles the width.css
properties were renamed for simplification:--page-grid-gutter-width-small
and--page-grid-gutter-width-large
were simplified into a single property,--page-grid-outside-margins
removing the size distinctions and allowing the sizing to be determined through media queries instead.--page-grid-col-width-small
and--page-grid-col-width-large
were simplified into a single property of--page-grid-column-widths
removing the size distinctions and allowing the sizing to be determined through media queries instead.Additionally, many more
css
properties were created/exposed to allow for better flexibility allowing teams to override various items as their needs require it. The full list ofcss
properties available are as follows:CSS
Property--page-grid-number-cols
--page-grid-column-gaps
--page-grid-row-gaps
--page-grid-outside-margins
--page-grid-column-widths
--page-grid-max-width
Though at times, various use cases may call for it, developer should be very careful overriding these as it can have detrimental effects.
Screenshots
Checklist