Skip to content
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

Combine AppIconButton and HelpIconButton #2716

Merged
merged 4 commits into from
Dec 9, 2024

Conversation

eason9487
Copy link
Member

@eason9487 eason9487 commented Dec 6, 2024

Changes proposed in this Pull Request:

The Button from @wordpress/components supports having an icon. However, the existence of AppIconButton makes it appear that there is a duplication of implementation between them.

Considering that the only use case for AppIconButton in the last four years has been HelpIconButton, this PR combines them into one.

Additional changes:

  • Fix the incorrect @return tag in JSDoc
  • Remove the forwarding of the rest props because all HelpIconButton use cases only set the eventContext prop.

Detailed test instructions:

  1. Go to any page that renders the HelpIconButton component
    • Edit free listings
    • Create campaign
    • Edit campaign
    • Edit store address
    • Onboarding flow
    • Ads-onboarding flow
  2. Check if HelpIconButton component is presented the same as before
    image
  3. Click on the button to see if it can open plugin's document page in a new webpage

Additional details:

💡 In the future, if there are more buttons need to set the position of icon relative to the top of the button text, it's preferred to extend AppButton to support iconPosition="top".

Changelog entry

@eason9487 eason9487 self-assigned this Dec 6, 2024
@github-actions github-actions bot added the changelog: dev Developer-facing only change. label Dec 6, 2024
Copy link

codecov bot commented Dec 6, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 63.4%. Comparing base (fd83778) to head (e806b2a).
Report is 5 commits behind head on develop.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##           develop   #2716   +/-   ##
=======================================
  Coverage     63.4%   63.4%           
=======================================
  Files          339     338    -1     
  Lines         5207    5203    -4     
  Branches      1275    1274    -1     
=======================================
- Hits          3302    3301    -1     
+ Misses        1730    1728    -2     
+ Partials       175     174    -1     
Flag Coverage Δ
js-unit-tests 63.4% <100.0%> (+<0.1%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
js/src/components/help-icon-button/index.js 50.0% <100.0%> (ø)

@eason9487 eason9487 requested a review from a team December 6, 2024 10:50
@eason9487 eason9487 force-pushed the dev/merge-help-icon-button branch from eddfad8 to e806b2a Compare December 9, 2024 02:34
Copy link
Member

@ianlin ianlin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for cleaning up the help icon button components. Confirmed all the help icons displayed correctly, redirected to the right page after clicking it, and the events were tracked accurately. LGTM.

@eason9487 eason9487 merged commit 21ea9a5 into develop Dec 9, 2024
8 checks passed
@eason9487 eason9487 deleted the dev/merge-help-icon-button branch December 9, 2024 03:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog: dev Developer-facing only change.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants