feat(Buttons): refactor code and fix spacing bug in loading buttonLink #1212
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Size stats
|
|
Accessibility report ℹ️ You can run this locally by executing |
|
Deploy preview for mistica-web ready! ✅ Preview Built with commit 71e4799. |
marcoskolodny
commented
Aug 23, 2024
There was a problem hiding this comment.
expected. The horizontal spacing of a loading ButtonLink was wrong
aweell
approved these changes
Aug 26, 2024
pladaria
reviewed
Aug 26, 2024
pladaria
approved these changes
Aug 26, 2024
tuentisre
pushed a commit
that referenced
this pull request
Sep 3, 2024
# [15.19.0](v15.18.0...v15.19.0) (2024-09-03) ### Bug Fixes * **Buttons:** avoid warnings related to change in order of react hooks ([#1229](#1229)) ([2dbc411](2dbc411)) * **i18n): Revert "feat(i18n:** improve texts sizes" because it is breaking ([#1226](#1226)) ([79eb4a4](79eb4a4)) * **Logo:** fix webpackChunkName magic comments ([#1214](#1214)) ([3d1f098](3d1f098)) * **Vivinho char:** vivinho char in headings being read as a separated heading ([#1209](#1209)) ([f0f5fb0](f0f5fb0)) ### Features * **Buttons:** refactor code and fix spacing bug in loading buttonLink ([#1212](#1212)) ([640e429](640e429)) * **i18n:** improve texts sizes ([#1204](#1204)) ([0345e7c](0345e7c)) * **Logo:** Refactor logo to improve bundle size and loading times ([#1210](#1210)) ([15b77cb](15b77cb)) * **NavigationBar, FunnelNavigationBar, MainNavigationBar:** add alternative variant ([#1200](#1200)) ([eef87ec](eef87ec))
|
🎉 This PR is included in version 15.19.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Issue: Link
🗒️ Note
While I was creating the default version of ButtonLink, I noticed that there is a lot of copy-paste in the button components. I've refactored the code to avoid this.
Also, after doing this refactor I saw that some screenshots tests were affected (ButtonLink with loading state). I investigated this, and I realized that my refactor unexpectedly fixed a bug we had in the ButtonLink component. When a Link in loading state is rendered, the horizontal spacing on the left/right of the button was incorrect (22.5px instead of 12px as the specs indicate).
A bit more about the bug: the loading content is positioned absolutely with left/right spacing of 12px, but the code also adds the original padding of the ButtonLink content (12px minus 1.5px of border, so an extra of 10.5 pixels). This adds up to the 22.5px we have today in prod, and this is wrong.
This bug can be reproduced with this snippet
This is NOT a breaking change. I've added the
smallprop for ButtonLink in order to avoid extra logic in the code. For now, the prop can only be set to true (basically, it does nothing). I'm working in another PR where I allow the prop to be false (part of the breaking changes for next major release).I don't see this as an issue, because it's a temporary thing and using the prop doesn't cause any side effect (it changes nothing). Adding this simplifies the code and trivializes the changes required for the major release.
✔️ About webapp
I've triggered mistica action against this branch in webapp and there are no failing tests or changes anywhere (results), so this refactor looks safe.