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

Remove duplicate settings button #5161

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

jennymul
Copy link
Contributor

@jennymul jennymul commented Nov 6, 2024

Description

Please write a summary of the changes, and please include relevant motivation and context. Remember to label your pull request properly; e.g. bug-fix, new-feature or docs.

Result

If you've made visual changes, please check the boxes below and include images showing the changes. Descriptions are appreciated.

  • Changes look good on both light and dark theme.
  • Changes look good with different viewports (mobile, tablet, etc.).
  • Changes look good with slower Internet connections.

Caution

Make sure your images do not contain any real user information.



Description Before After
...

image

image

Testing

  • [ X] I have thoroughly tested my changes.

@jennymul jennymul requested a review from falbru November 6, 2024 18:04
@jennymul jennymul self-assigned this Nov 6, 2024
Copy link

vercel bot commented Nov 6, 2024

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

1 Skipped Deployment
Name Status Preview Comments Updated (UTC)
lego-bricks-storybook ⬜️ Ignored (Inspect) Nov 6, 2024 6:06pm

@github-actions github-actions bot added the review-needed Pull requests that need review label Nov 6, 2024
@jennymul jennymul force-pushed the fix-duplicate-settings-button branch from a705278 to 4a45f63 Compare November 6, 2024 18:06
Copy link
Member

@ivarnakken ivarnakken left a comment

Choose a reason for hiding this comment

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

Very nice! 😄

>
<Icon
iconNode={<SettingsIcon />}
size={22}
Copy link
Member

Choose a reason for hiding this comment

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

22 is a bit large when it is inside a button - perhaps we could do something like 19 (like we do for most other buttons with icons)

iconNode={<SettingsIcon />}
size={22}
className={styles.settingsIcon}
/>{' '}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
/>{' '}
/>

Copy link
Contributor

@Viljen789 Viljen789 left a comment

Choose a reason for hiding this comment

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

It looks good as far as the code goes, functionality wise at least!

Copy link
Contributor

@ch0rizo ch0rizo left a comment

Choose a reason for hiding this comment

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

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
review-needed Pull requests that need review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants