Part of #17670: Replace Typography with Text component in CancelSpeedupPopover #18638

kremalicious · 2023-04-18T13:16:13Z

Part of #17670.

Switch Typography components with Text component within the CancelSpeedupPopover. Additionally, created story for it. And kicked out some redundant Box and div as they were not doing anything except for bloating the DOM, although still a bit confused about all the nested Box usage even within the Text component. Also boxProps are redundant with new Text component as far as I can see.

Also nicer balanced spacing between text, divider, and the rest.

As for the story, was a bit tricky getting the right context providers and their respective test data. In end had to slap in 3 providers and used mock data from the ./test folder instead of ./storybook folder. If this is not desired, I could look into figuring out which exact mock data needs to be added to the mock data files under ./storybook. And if anybody knows an easier way to get the story for this component running, I'm all ears!

Finally, also saw this code suggestion by @georgewrmarshall on a former closed PR but I do not agree, as, semantically, a p is perfectly fine for the modal description up there and an h6 should not directly follow an h2.

Screenshots/Screencaps

Before

After

Pre-merge author checklist

I've clearly explained:
- What problem this PR is solving
- How this problem was solved
- How reviewers can test my changes
Sufficient automated test coverage has been added

Pre-merge reviewer checklist

Manual testing (e.g. pull and build branch, run in browser, test code being changed)
PR is linked to the appropriate GitHub issue
IF this PR fixes a bug in the release milestone, add this PR to the release milestone

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.

github-actions · 2023-04-18T13:16:29Z

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.

georgewrmarshall

Hey @kremalicious, this is excellent! Thank you very much for your contribution. The story is outstanding 💯 I've made some more suggestions that are out of the scope of the original task but they are small updates compared to the extra work you've put in on the story and think it will further improve this component. As for the h6 suggestion you are totally correct! Unfortunately the old Typography component automatically set the semantic tags so those had to be set to ensure e2e tests would pass.

ui/components/app/cancel-speedup-popover/cancel-speedup-popover.js

ui/components/app/cancel-speedup-popover/index.scss

ui/components/app/cancel-speedup-popover/cancel-speedup-popover.js

ui/components/app/cancel-speedup-popover/cancel-speedup-popover.stories.js

georgewrmarshall

Looking good! Seems like e2e tests are failing but I'm not sure why

ui/components/app/cancel-speedup-popover/cancel-speedup-popover.js

ui/components/app/cancel-speedup-popover/index.scss

* addresses MetaMask#18651 * taken from MetaMask#18752 as suggested in MetaMask#18638 (comment)

ui/components/component-library/text/text.scss

georgewrmarshall

Looking great! One small suggestion and then I will approve 👍

ui/components/app/cancel-speedup-popover/cancel-speedup-popover.js

georgewrmarshall

LGTM! 💯 Thanks for your contribution @kremalicious 🙏

georgewrmarshall · 2023-04-28T10:02:32Z

@brad-decker I'm not familiar with this build issue have you seen it before? Don't seem to see it on any other PRs and have logged out and back in

kremalicious · 2023-04-28T11:24:39Z

I would have a small additional change after seeing that the info tooltip is not consistent with the other tooltip from the "Gas" headline. Have a change which would make it like so:

Before:

After:

Everybody cool if I push this change in here? It's adding TextVariant.bodyXs to the respective components.

brad-decker · 2023-04-28T12:25:36Z

@georgewrmarshall I think a fluke, i am rerunning

georgewrmarshall · 2023-04-28T23:21:09Z

Hey @kremalicious, I also noticed the text size change and was ok with it because I think larger font size helps with accessibility and our new Tooltip will be at this size. Ok with either as this would be consistent with current tooltips.

brad-decker · 2023-05-02T14:54:05Z

@kremalicious if you have revoked circleCI access to your GitHub it may prevent your fork from running our test suite. I just updated your branch again to see if this will resolve itself if not you may want to investigate from your end on a permission level. To our knowledge no other contributors are presently experiencing CI issues that aren't relevant to their changes or tests we know to be flaky. In this case the entire pipeline is failing to run. NVM looks like it ran this time?

* replace CSS with props styling * use `Button` from `component-library` * tooltip HTML refactor with `component-library` components * remove whitespace in story

* addresses MetaMask#18651 * taken from MetaMask#18752 as suggested in MetaMask#18638 (comment)

georgewrmarshall

Hey @kremalicious, I went to the liberty of resolving the conflicts and pushing up the changes. I hope you don't mind. Definitely one of the most thorough and well written contributions I've seen in a while. Thank you very much for your time, care and effort on this one 💯 🙏

ui/components/app/cancel-speedup-popover/cancel-speedup-popover.js

georgewrmarshall · 2023-05-18T00:07:30Z

Hey @kremalicious, just wanted to check in. It looks like this PR is almost there. Did you still want to work on it? If so, that's great! Let us know if you need any assistance or if there are any blockers. If you're unable to continue working on it or haven't had a chance to address it, no worries! I can take it from here and carry it forward. Cheers!

georgewrmarshall

LGTM! Not sure why tests are passing will try re-run them otherwise I may need some support from @brad-decker

pulled branch and checked storybook

garrettbear · 2023-06-01T17:08:47Z

LGTM! I updated the branch to see if the jest error will pass (which it should be)

georgewrmarshall · 2023-06-01T21:16:30Z

Looks like tests are failing legitimately this time and some investigation will be needed it may be that the semantic html needs to be updated in the e2e tests

georgewrmarshall · 2023-06-19T20:30:52Z

ui/components/app/cancel-speedup-popover/cancel-speedup-popover.test.js

@@ -125,7 +125,7 @@ describe('CancelSpeedupPopover', () => {
  it('information tooltip should contain the correct text if editGasMode is cancel', async () => {
    await act(async () => render());
    expect(
-      InfoTooltip.mock.calls[0][0].contentText.props.children[0],
+      InfoTooltip.mock.calls[0][0].contentText.props.children[0].props.children,


I hate this but don't have time right now to improve the tests for this

brad-decker

More questions then feedback. Possibly just questions for @georgewrmarshall

brad-decker · 2023-06-20T22:12:29Z

ui/components/app/cancel-speedup-popover/cancel-speedup-popover.js

        >
          {t('cancelSpeedUpLabel', [
-            <strong key="cancelSpeedupReplace">{t('replace')}</strong>,
+            <strong key="cancelSpeedupCancel">{t('replace')}</strong>,


Question, why was the key changed on this? Also...why is the key needed? This might be a question for @georgewrmarshall

Good question! This may have been a mistake. I'll remove it

Ah right I'm not super familiar with locales but because it's in an array it needs a key

brad-decker · 2023-06-20T22:13:25Z

ui/components/app/cancel-speedup-popover/cancel-speedup-popover.js

+              <>
+                <Text variant={TextVariant.bodySm}>
+                  {t('cancelSpeedUpTransactionTooltip', [
+                    editGasMode === EditGasModes.cancel
+                      ? t('cancel')
+                      : t('speedUp'),
+                  ])}
+                </Text>
+                <ButtonLink
+                  variant={TextVariant.bodySm}
+                  href="https://community.metamask.io/t/how-to-speed-up-or-cancel-transactions-on-metamask/3296"
+                  target="_blank"
+                >
+                  {t('learnMoreUpperCase')}
+                </ButtonLink>
+              </>


Is the choice to remove the in favor of a fragment related to the issue?

Probably not particularly related to the issue but could have some minor performance benefits as there are no utility props being used on the Box so could save a small amount of render time?

georgewrmarshall

LGTM! Thanks for your contribution @kremalicious 🙏

kremalicious requested a review from a team as a code owner April 18, 2023 13:16

kremalicious requested a review from OGPoyraz April 18, 2023 13:16

georgewrmarshall reviewed Apr 19, 2023

View reviewed changes

georgewrmarshall added the team-design-system All issues relating to design system in Extension label Apr 19, 2023

georgewrmarshall reviewed Apr 19, 2023

View reviewed changes

ui/components/app/cancel-speedup-popover/cancel-speedup-popover.stories.js Outdated Show resolved Hide resolved

georgewrmarshall reviewed Apr 21, 2023

View reviewed changes

ui/components/app/cancel-speedup-popover/cancel-speedup-popover.js Outdated Show resolved Hide resolved

DDDDDanica reviewed Apr 25, 2023

View reviewed changes

ui/components/app/cancel-speedup-popover/index.scss Outdated Show resolved Hide resolved

kremalicious added a commit to kremalicious/metamask-extension that referenced this pull request Apr 25, 2023

strong tag support within Text component

fa564e3

* addresses MetaMask#18651 * taken from MetaMask#18752 as suggested in MetaMask#18638 (comment)

DDDDDanica previously approved these changes Apr 27, 2023

View reviewed changes

brad-decker suggested changes Apr 27, 2023

View reviewed changes

ui/components/component-library/text/text.scss Outdated Show resolved Hide resolved

kremalicious dismissed DDDDDanica’s stale review via 5ceac74 April 27, 2023 16:23

georgewrmarshall reviewed Apr 28, 2023

View reviewed changes

ui/components/app/cancel-speedup-popover/cancel-speedup-popover.js Outdated Show resolved Hide resolved

georgewrmarshall approved these changes Apr 28, 2023

View reviewed changes

DDDDDanica previously approved these changes Apr 28, 2023

View reviewed changes

georgewrmarshall previously approved these changes Apr 28, 2023

View reviewed changes

georgewrmarshall closed this May 5, 2023

github-actions bot locked and limited conversation to collaborators May 5, 2023

georgewrmarshall reopened this May 5, 2023

kremalicious added 6 commits May 5, 2023 11:12

create story

1b41e2c

replace Typography with Text component

fd959f5

review changes

a4b1d55

* replace CSS with props styling * use `Button` from `component-library` * tooltip HTML refactor with `component-library` components * remove whitespace in story

strong tag support within Text component

568a932

* addresses MetaMask#18651 * taken from MetaMask#18752 as suggested in MetaMask#18638 (comment)

replace strong with new Text as="strong"

e0c6db3

remove unneccessary css from fa564e3

62cc742

georgewrmarshall requested review from DDDDDanica and georgewrmarshall May 5, 2023 01:58

georgewrmarshall previously approved these changes May 5, 2023

View reviewed changes

DDDDDanica reviewed May 5, 2023

View reviewed changes

ui/components/app/cancel-speedup-popover/cancel-speedup-popover.js Outdated Show resolved Hide resolved

restrore proper spacing between elements

23626ad

kremalicious dismissed georgewrmarshall’s stale review via 23626ad May 6, 2023 09:26

Merge branch 'develop' into feature/17670-cancel-speedup-popover

31992b8

kremalicious added 2 commits May 20, 2023 14:07

Merge branch 'develop' into feature/17670-cancel-speedup-popover

cfc1b49

Merge branch 'develop' into feature/17670-cancel-speedup-popover

ac5922e

georgewrmarshall previously approved these changes Jun 1, 2023

View reviewed changes

Merge branch 'develop' into feature/17670-cancel-speedup-popover

bc03fb3

garrettbear previously approved these changes Jun 1, 2023

View reviewed changes

Merge branch 'develop' into feature/17670-cancel-speedup-popover

e8074f0

georgewrmarshall dismissed stale reviews from garrettbear and themself via 0fb6e19 June 19, 2023 19:03

Quick fix for test

73307fd

georgewrmarshall force-pushed the feature/17670-cancel-speedup-popover branch from 0fb6e19 to 73307fd Compare June 19, 2023 20:30

georgewrmarshall reviewed Jun 19, 2023

View reviewed changes

Merge branch 'develop' into feature/17670-cancel-speedup-popover

a6fb0dd

brad-decker reviewed Jun 20, 2023

View reviewed changes

georgewrmarshall force-pushed the feature/17670-cancel-speedup-popover branch from bdd0899 to a6fb0dd Compare June 20, 2023 23:12

georgewrmarshall added 2 commits June 20, 2023 16:14

Adding key

a0f3ea7

Merge branch 'develop' into feature/17670-cancel-speedup-popover

bdd3844

brad-decker approved these changes Jun 26, 2023

View reviewed changes

georgewrmarshall approved these changes Jun 26, 2023

View reviewed changes

georgewrmarshall merged commit 113fdc1 into MetaMask:develop Jun 26, 2023
50 checks passed

georgewrmarshall added the external-contributor label Oct 10, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Part of #17670: Replace Typography with Text component in CancelSpeedupPopover #18638

Part of #17670: Replace Typography with Text component in CancelSpeedupPopover #18638

kremalicious commented Apr 18, 2023 •

edited

github-actions bot commented Apr 18, 2023

georgewrmarshall left a comment

georgewrmarshall left a comment

georgewrmarshall left a comment

georgewrmarshall left a comment

georgewrmarshall commented Apr 28, 2023

kremalicious commented Apr 28, 2023 •

edited

brad-decker commented Apr 28, 2023

georgewrmarshall commented Apr 28, 2023

brad-decker commented May 2, 2023 •

edited

georgewrmarshall left a comment

georgewrmarshall commented May 18, 2023

georgewrmarshall left a comment

garrettbear commented Jun 1, 2023

georgewrmarshall commented Jun 1, 2023

georgewrmarshall Jun 19, 2023

brad-decker left a comment

brad-decker Jun 20, 2023

georgewrmarshall Jun 20, 2023

georgewrmarshall Jun 20, 2023

brad-decker Jun 20, 2023

georgewrmarshall Jun 20, 2023

brad-decker Jun 26, 2023

georgewrmarshall left a comment

Part of #17670: Replace Typography with Text component in CancelSpeedupPopover #18638

Part of #17670: Replace Typography with Text component in CancelSpeedupPopover #18638

Conversation

kremalicious commented Apr 18, 2023 • edited

Screenshots/Screencaps

Before

After

Pre-merge author checklist

Pre-merge reviewer checklist

github-actions bot commented Apr 18, 2023

georgewrmarshall left a comment

Choose a reason for hiding this comment

georgewrmarshall left a comment

Choose a reason for hiding this comment

georgewrmarshall left a comment

Choose a reason for hiding this comment

georgewrmarshall left a comment

Choose a reason for hiding this comment

georgewrmarshall commented Apr 28, 2023

kremalicious commented Apr 28, 2023 • edited

brad-decker commented Apr 28, 2023

georgewrmarshall commented Apr 28, 2023

brad-decker commented May 2, 2023 • edited

georgewrmarshall left a comment

Choose a reason for hiding this comment

georgewrmarshall commented May 18, 2023

georgewrmarshall left a comment

Choose a reason for hiding this comment

garrettbear commented Jun 1, 2023

georgewrmarshall commented Jun 1, 2023

georgewrmarshall Jun 19, 2023

Choose a reason for hiding this comment

brad-decker left a comment

Choose a reason for hiding this comment

brad-decker Jun 20, 2023

Choose a reason for hiding this comment

georgewrmarshall Jun 20, 2023

Choose a reason for hiding this comment

georgewrmarshall Jun 20, 2023

Choose a reason for hiding this comment

brad-decker Jun 20, 2023

Choose a reason for hiding this comment

georgewrmarshall Jun 20, 2023

Choose a reason for hiding this comment

brad-decker Jun 26, 2023

Choose a reason for hiding this comment

georgewrmarshall left a comment

Choose a reason for hiding this comment

kremalicious commented Apr 18, 2023 •

edited

kremalicious commented Apr 28, 2023 •

edited

brad-decker commented May 2, 2023 •

edited