-
Notifications
You must be signed in to change notification settings - Fork 5k
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
Migrate remaining design system constants to enums #19133
Conversation
CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes. |
I have read the CLA Document and I hereby sign the CLA |
No dependency changes detected. Learn more about Socket for GitHub ↗︎ 👍 No dependency changes detected in pull request Pull request alert summary
|
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.
Hey @nazrul7711, thanks for your contribution. This is looking really great! I've left a few small suggestions. It would also be great if you could update the PR description according to the template provided. This really helps with review.
I wasn't sure if we would have to update the box.d.ts
file with a union type of the Display
enum as well as the DISPLAY
const? There doesn't seem to be any errors in the TypeScript files I was looking at which I found interesting I suppose because it is just looking for the string or an array of strings.
hi @georgewrmarshall I have made the modifications , kindly review it. |
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.
Hey @nazrul7711, I've updated the PR title and description so the PR is a little more clear. Some of it was still commented out. I've also realized that I missed SEVERITIES
in the issue(now updated). Could you also create an enum for Severity
. I don't think we need one for RESIZE
as it's used for a single component that will likely be migrated to TypeScript and have it's own types.
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.
Hey @nazrul7711, I rebased this PR to remove the yarn.lock
changes and made sure that the deprecated constants still work by keeping the unit tests for them. This was kind of blocking some other TS migration work so I really appreciate your timing on this and hope you don't mind my updates. LGMT 🔥
Codecov Report
@@ Coverage Diff @@
## develop #19133 +/- ##
===========================================
- Coverage 69.54% 69.53% -0.01%
===========================================
Files 960 960
Lines 36583 36583
Branches 9493 9493
===========================================
- Hits 25439 25437 -2
- Misses 11144 11146 +2
|
Explanation
Currently there are some remaining design system constants that need to be migrated to enums. This PR creates enums for the remaining constants. It also deprecates the constants in favour of the enums and points to a good first issue to encourage contributors to replace them.
DISPLAY
,FLEX_DIRECTION
,FLEX_WRAP
,BLOCK_SIZES
,SEVERITIES
have been createdBox
component, documentation and tests updated to favour enums but support constants through unit testingScreenshots/Screencaps
Before
before.mov
After
after.mov
Manual Testing Steps
DISPLAY
,FLEX_DIRECTION
,FLEX_WRAP
,BLOCK_SIZES
,SEVERITIES
Pre-merge author checklist
Pre-merge reviewer checklist
If further QA is required (e.g. new feature, complex testing steps, large refactor), add the
Extension QA Board
label.In this case, a QA Engineer approval will be be required.