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.

Sign up for GitHub

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

Jump to bottom

chore: Remove ButtonIcon #181

Merged
merged 4 commits into from
Jul 21, 2022
Merged

chore: Remove ButtonIcon #181

merged 4 commits into from
Jul 21, 2022

Conversation

lucasfp13
Copy link
Contributor

@lucasfp13 lucasfp13 commented Jul 19, 2022 •
edited
Loading

What's the purpose of this pull request?

This PR intends to remove the ButtonIcon component and all its references. It will be replaced by the regular Button with specific data attributes for this case.

How to test it?

Everything should look the same after these changes.

Checklist

Changelog

  • Added an entry in the CHANGELOG.md at the beginning of its due section. The latest version should comes first.
  • Added the PR number with the PR link at the entry in the CHANGELOG.md. E.g., New items in the pull_request_template.md (#4)

PR Description

  • Added a label according to the PR goal - Breaking change, Enhancement, Bug or Chore.
  • Added the component, hook, or pathname in-between backticks (``) - If applicable. E.g., ComponentName component.
  • Identified the function or parameter in the PR - If applicable. E.g., useWindowDimensions hook.

Documentation

  • PR description

@lucasfp13 lucasfp13 added the Chore General tasks. label Jul 19, 2022
@lucasfp13 lucasfp13 self-assigned this Jul 19, 2022
@vercel
Copy link

vercel bot commented Jul 19, 2022 •
edited
Loading

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated
nextjs-store-storybook ✅ Ready (Inspect) Visit Preview Jul 21, 2022 at 5:36PM (UTC)

@vercel vercel bot temporarily deployed to Preview July 19, 2022 21:06 Inactive
@vtex-sites
Copy link

vtex-sites bot commented Jul 19, 2022 •
edited
Loading

Preview is ready

This pull request generated a Preview

👀   Preview: https://preview-181--nextjs.preview.vtex.app
🔬   Go deeper by inspecting the Build Logs
📝   based on commit 342388a

@vtex-sites
Copy link

vtex-sites bot commented Jul 19, 2022 •
edited
Loading

Lighthouse Reports

Here are the Lighthouse reports of this Pull Request

📝 Based on commit 342388a

Lighthouse Report by page
📎   /
📎   /apple-magic-mouse/p
📎   /office

filipewl
filipewl reviewed Jul 19, 2022
View reviewed changes
Copy link
Contributor

@filipewl filipewl left a comment

Choose a reason for hiding this comment

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

The changes look good to me, but I noticed the following visual changes to the position of the alert's close button & regionalization:

Before After
CleanShot 2022-07-19 at 18 15 42@2x CleanShot 2022-07-19 at 18 15 31@2x

src/components/ui/Button/Button.tsx Outdated
@@ -62,7 +62,9 @@ function Button({
disabled={disabled}
{...props}
>
{iconPosition === 'left' && <UIIcon component={icon} />}
{(!iconPosition || iconPosition === 'left') && (
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need this same kind of treatment for the icon's right position?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, we don't. I did this only to support the case the user doesn't set iconPosition property, i.e. for the icon button cases that we don't need set any position

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Noticed that this was causing this issue here, so I'm thinking in another way

eduardoformiga
eduardoformiga reviewed Jul 19, 2022
View reviewed changes
src/components/ui/Button/Button.tsx Outdated
@@ -62,7 +62,9 @@ function Button({
disabled={disabled}
{...props}
>
{iconPosition === 'left' && <UIIcon component={icon} />}
{(!iconPosition || iconPosition === 'left') && (
Copy link
Member

Choose a reason for hiding this comment

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

If we don't pass the iconPosition prop should we render the UIIcon? I think I didn't get it well.

Copy link
Contributor

Choose a reason for hiding this comment

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

this is confusing. I think we shouldn't render UIIcon if not passed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the UIIcon handles the case the icon doesn't exist and displays nothing. I explained here my intention there

@hellofanny
Copy link
Contributor

Nicee! 🌟 I've noticed a visual change:

There is an extra padding on the left side. I guess it's because of the column-gap. Since there is no icon element displayed, I think we should not render it when no icon provided. Can you check it pls? 🚀

Before After
Screenshot (4) Screenshot (5)

@vercel vercel bot temporarily deployed to Preview July 20, 2022 18:19 Inactive
@lucasfp13 lucasfp13 force-pushed the chore/remove-button-icon branch from 569cb69 to 116bf8f Compare July 20, 2022 18:27
eduardoformiga
eduardoformiga approved these changes Jul 20, 2022
View reviewed changes
src/components/ui/Button/Button.tsx
@@ -51,6 +51,8 @@ function Button({
disabled,
...props
}: ButtonProps) {
const isButtonIcon = icon && !iconPosition
Copy link
Member

Choose a reason for hiding this comment

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

I like it!

@vercel vercel bot temporarily deployed to Preview July 20, 2022 18:28 Inactive
filipewl
filipewl approved these changes Jul 20, 2022
View reviewed changes
Copy link
Contributor

@filipewl filipewl left a comment

Choose a reason for hiding this comment

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

Approved!

@hellofanny hellofanny self-requested a review July 21, 2022 05:22
hellofanny
hellofanny approved these changes Jul 21, 2022
View reviewed changes
Copy link
Contributor

@hellofanny hellofanny left a comment

Choose a reason for hiding this comment

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

Uhull! I've checked it again, this issue is solved and everything looks great 🌟

@lucasfp13 lucasfp13 force-pushed the chore/remove-button-icon branch from 116bf8f to 342388a Compare July 21, 2022 17:34
@vercel vercel bot temporarily deployed to Preview July 21, 2022 17:36 Inactive
@lucasfp13 lucasfp13 merged commit a752a9e into main Jul 21, 2022
@lucasfp13 lucasfp13 deleted the chore/remove-button-icon branch July 21, 2022 17:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@eduardoformiga eduardoformiga eduardoformiga approved these changes

@hellofanny hellofanny hellofanny approved these changes

@filipewl filipewl filipewl approved these changes

Assignees

@lucasfp13 lucasfp13

Labels
Chore General tasks.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@lucasfp13 @hellofanny @filipewl @eduardoformiga