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

ActionIcon: Fix mismatch between demo apparent code and real code #2189

Merged
merged 3 commits into from Aug 21, 2022
Merged

ActionIcon: Fix mismatch between demo apparent code and real code #2189

merged 3 commits into from Aug 21, 2022

Conversation

slk333
Copy link
Contributor

@slk333 slk333 commented Aug 21, 2022

Please see the screenshots and the videos in the discord thread: https://discord.com/channels/854810300876062770/1010668931842855064

This PR is more of a "Expose the problem to the maintainers" PR than anything, even though it does fix the mismatch

The demo real code was changing both the ActionIcon's size and the embedded Icon's size.
While this seems to be desirable, it was not reflected in the apparent demo code.

There are two way to "fix" this. One simple way, and one difficult way

  1. (Easy way) Make the real code match the apparent code: do not change the embedded Icon's size when the slider moves.
  2. (Hard way) Make the apparent code match the real code: continue to change the embedded Icon's size when the slider moves, but reflect that in the code template in real time as well.

This PR takes the first option, as it is the most straightforward, but I believe the second option might be better. It would require extensive string manipulation though to change the template code and I want to leave that to the maintainer discretion

Before:
Screenshot 2022-08-21 at 11 23 18

After:
Screenshot 2022-08-21 at 11 20 31

The real code was changing both the ActionIcon size and the embedded Icon size
@rtivital
Copy link
Member

The demo should include icon size instead.

@slk333
Copy link
Contributor Author

slk333 commented Aug 21, 2022

The demo should include icon size instead.

I changed the code to follow your suggestion 👍

Screen.Recording.2022-08-21.at.15.33.45.mov

@rtivital rtivital merged commit 1480d83 into mantinedev:master Aug 21, 2022
@rtivital
Copy link
Member

Thanks!

nmay231 pushed a commit to nmay231/mantine that referenced this pull request Aug 22, 2022
…e in ActionIcon demo (mantinedev#2189)

* ActionIcon:  Fix mismatch between demo apparent code and real code

The real code was changing both the ActionIcon size and the embedded Icon size

* apparent code matches real code instead

* fix prettier
nmay231 pushed a commit to nmay231/mantine that referenced this pull request Aug 28, 2022
…e in ActionIcon demo (mantinedev#2189)

* ActionIcon:  Fix mismatch between demo apparent code and real code

The real code was changing both the ActionIcon size and the embedded Icon size

* apparent code matches real code instead

* fix prettier
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants