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 |
georgewrmarshall
added
the
team-design-system
| <Text | ||
| variant={TextVariant.headingSm} | ||
| as="h4" | ||
| fontWeight={FontWeight.Bold} |
There was a problem hiding this comment.
Shouldn't need the bold weight as headingSm is already bold?
| fontWeight={FontWeight.Bold} |
There was a problem hiding this comment.
I made the suggested changes but the same tests are failing again :/
Sorry for the inconvenience.
…xt css strong tag styles
georgewrmarshall
force-pushed
the
Part-of-18714-and-17670
branch
from
bd4194e
to
3106705
Compare
|
|
||
| &__bold { | ||
| color: var(--color-text-default); | ||
| } |
There was a problem hiding this comment.
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.
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.
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.jsas well as fixes strong tag issue in Text componentPart of
Screenshots/Screencaps
Before
After
Manual Testing Steps
yarn storybookSetApproveForAllWarningstoryPre-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 Boardlabel.In this case, a QA Engineer approval will be be required.