-
Notifications
You must be signed in to change notification settings - Fork 83
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
KCardGrid part 3: Add advanced grid configuration and loading skeletons #796
Conversation
76ccd79
to
c1debf5
Compare
This simplifies config object format and related language around breakpoints. Prepares ground for `layoutOverride` and `skeletonsConfig` public API that will have the same format.
Also removes progressive loading experience from card placeholder since loading state is now handled by loading skeletons and the previous logic causes unneccessary hiccup when the placeholder is displayed very briefly before the image.
to simpler name that's also consistent with the other grid composable's name.
and remove duplicate value.
to allow text in <code> tag be wrapped.
For code reviewers, I don't think you need to review the large diff on documentation pages - rather just preview the generated documentation. One of the maintenance tasks I am planning to do in a follow-up PR is to break doc pages and examples to sub-components internally so it's easier to updated in the future. |
Linking feedback from designers - Slack thread |
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.
Main question is just about the form of the layout overrides.
Latest updates
I think this is all I could complete here at this point. We still need to decide on #796 (comment) - I will leave to @rtibbles's consideration whether it's blocking or if it can be addressed as a follow-up if we figure out some changes are needed. |
I don't think my comments here are blocking - I think by using the grid and skeletons in practice, we may discover that there are some sensible defaults we end up using, and the more advanced props format doesn't matter as much as it is rarely used - or we come up with a brainwave of a better way. |
All is ready from my side @rtibbles |
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.
I say we merge this, get it into an RC and take the API for a few practice laps!
Description
Adds new features to
KCardGrid
:layoutOverride
skeletonsConfig
debug
prop to help with skeletons configurationThese APIs were previously discussed and agreed on during the beta stage.
See commit messages for details.
Changelog
Description: Adds an option to override
KCardGrid
base layouts partially or completely via the new proplayoutOverride
Products impact: new API
Addresses: Allows advanced grids customization
Components:
KCardGrid
Breaking: no
Impacts a11y: no
Guidance: -
Description: Adds loading skeletons to
KCardGrid
and a way to configure them via the new propskeletonsConfig
Products impact: new API
Addresses: Ensures smooth loading experience
Components:
KCardGrid
Breaking: no
Impacts a11y: no
Guidance: -
Steps to test
Testing checklist
If there are any front-end changes, before/after screenshots are includedReviewer guidance
Comments
I got stuck on some issues with mocking composables in KDS that prevented me from completing unit tests. This is being discussed and I plan to follow-up later, together with some other maintenance tasks I am going to do.