-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
Part of #18714 and #17670:set-approval-for-all-warnings #19115
Part of #18714 and #17670:set-approval-for-all-warnings #19115
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. |
Codecov Report
@@ Coverage Diff @@
## develop #19115 +/- ##
===========================================
- Coverage 69.54% 69.54% -0.00%
===========================================
Files 960 960
Lines 36575 36574 -1
Branches 9497 9496 -1
===========================================
- Hits 25434 25433 -1
Misses 11141 11141
|
Hey @georgewrmarshall could you please review this PR? It shows no problems in the code, all ci tests passed aswell |
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! Code coverage failing is because of linting formatting change
<Text | ||
variant={TextVariant.headingSm} | ||
as="h4" | ||
fontWeight={FontWeight.Bold} |
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.
Shouldn't need the bold weight as headingSm is already bold?
fontWeight={FontWeight.Bold} |
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 made the suggested changes but the same tests are failing again :/
Sorry for the inconvenience.
…xt css strong tag styles
bd4194e
to
3106705
Compare
|
||
&__bold { | ||
color: var(--color-text-default); | ||
} |
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.
Removed styles and made all text text/default color
@@ -33,7 +33,7 @@ $text-variants: ( | |||
color: var(--color-text-default); | |||
font-family: var(--font-family-sans); | |||
|
|||
> strong { | |||
strong { |
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.
Fixes issue where only the direct strong tag child would be bold font-weight. That meant if it was wrapped in a span
which many of the translations are they would not receive the styles properly.
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 @dhruvv173, by comparing the before and after screenshots in your PR description I was able to spot some issues with our Text
component and strong tag. I went to the liberty of updating it in your PR I hope you don't mind. I've also updated some more deprecated components with the new ones as well as updated the PR description and before/after screenshots to the most recent. Thanks so much for your contribution!!
Hey @georgewrmarshall, apologies for the mistakes from my end and thanks a lot for your input and help. You've been very helpful and polite while handling issues, really appreciate it 🙏 ❤️ |
Hey @dhruvv173, there were no mistakes on your end. These were just further improvements that could be made that weren't outlined in the original issue. It was your added screenshots that helped with these improvements so thanks for making sure your PR description was detailed. It makes a lot of difference! 👍 |
Explanation
Replaces deprecated Typography and Button components in
ui\components\app\set-approval-for-all-warning\set-approval-for-all-warning.js
as well as fixes strong tag issue in Text componentPart of
Screenshots/Screencaps
Before
After
Manual Testing Steps
yarn storybook
SetApproveForAllWarning
storyPre-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.