-
Notifications
You must be signed in to change notification settings - Fork 1
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(grid): create grid system [HEYUI-50] #26
Conversation
Pull Request Test Coverage Report for Build 2369629900Details
💛 - Coveralls |
Compiled a new version demo. |
Coverage report
Show new covered files 🐣
Test suite run success53 tests passing in 8 suites. Report generated by 🧪jest coverage report action from 3037845 |
Compiled a new version demo. |
Compiled a new version demo. |
Compiled a new version demo. |
Compiled a new version demo. |
ec544ff
to
baaccc6
Compare
Compiled a new version demo. |
Compiled a new version demo. |
Compiled a new version demo. |
Compiled a new version demo. |
Compiled a new version demo. |
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.
do not forgot add README, just check the example for the button in the main branch.
Added extra media queries: | Name | Value | | ------------- | --------------- | | `--mobile-s` | 320px to 100% | | `--mobile-m` | 375px to 100% | | `--mobile-l` | 412px to 100% | | `--mobile` | 0 to 599px | | `--tablet-s` | 600px to 100% | | `--tablet-m` | 768px to 100% | | `--tablet-l` | 860px to 100% | | `--tablet` | 600px to 1023px | | `--desktop-s` | 1024px to 100% | | `--desktop-m` | 1280px to 100% | | `--desktop-l` | 1440px to 100% | | `--desktop` | 1024px to 100% |
Compiled a new version demo. |
Compiled a new version demo. |
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.
PR looks good apart from one comment why did we prioritise flex over css grid for these components
max-width: 100%; | ||
min-height: 1px; | ||
flex-basis: 0; | ||
flex-grow: 1; |
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.
Can we use the short-hand property as used below
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.
The ability of the CSS flex rule is enough for the easy and fast control of the simple grid system.
You can check it out in the investigation ticket: https://heycar.atlassian.net/browse/HEYUI-49
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.
Can we use the short-hand property as used below
I didn't get this comment. could you explain pls ?
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.
flex: [flex-grow] [flex-shrink] [flex-basis]
I see everywhere in the PR shorthands used here I saw flex-grow and basis separately applied.
Compiled a new version demo. |
- @heycar-uikit/[email protected] - @heycar-uikit/[email protected] - @heycar-uikit/[email protected] - @heycar-uikit/[email protected] - @heycar-uikit/[email protected] - @heycar-uikit/[email protected]
Compiled a new version demo. |
# [1.4.0](v1.3.0...v1.4.0) (2022-07-14) ### Features * **grid:** The first implementation the grid package ([#26](#26)) ([807290a](807290a))
🎉 This PR is included in version 1.4.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Pull Request
Description
Type of change
How do I test this
Checklist
Did you remember to take care of the following?
npm i
– for new NPM dependencies.npm run lint
- to check for linting issuesnpm run test
- to run unit testsnpm run test:screenshots
- to run snapshot testsNew Feature / Bug Fix
Thanks for contributing!