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

Update snippet based on selection #32

Merged
merged 10 commits into from
Aug 28, 2023
Merged

Conversation

danirabbit
Copy link
Member

@danirabbit danirabbit commented Feb 22, 2023

Fixes #12

Copy link

@jeremypw jeremypw left a comment

Choose a reason for hiding this comment

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

  1. Clicking on a symbolic icon child after a color icon results in two selected items. Even though one is unfocused this still looks wrong/confusing to me.
    Screenshot from 2023-03-02 18 57 34

  2. If the icon name is changed after selecting an icon size and type, the icon size and type reverts to the default of 24px color. I think the selected size and type should persist (if available).

  3. After selecting an icon size for "emblem-documents" then change to a different icon name the icon sizes are no longer selectable. This can happen with other icons but the exact reason is unclear - possibly due to unavailable color icons?

  4. Emote icons do not have a "24px" version; nevertheless the code snippet is for 24px

@danirabbit
Copy link
Member Author

@jeremypw

1, 3, and 4. Fixed!
2. I think this would require a substantial rewrite since currently when we change icons we destroy all the flowbox children and rebuild them. I think it's a big enough change that it should be left for a future branch

@danirabbit danirabbit requested a review from jeremypw August 23, 2023 20:22
@danirabbit danirabbit mentioned this pull request Aug 24, 2023
@zeebok
Copy link

zeebok commented Aug 24, 2023

This looks good to me, and addresses the issues Jeremy spotted. If you like I can approve.

@danirabbit danirabbit dismissed jeremypw’s stale review August 28, 2023 19:51

Haven't heard back in a couple weeks and someone else confirmed this fixed raised issues

@danirabbit danirabbit merged commit 195cc4b into main Aug 28, 2023
@danirabbit danirabbit deleted the danirabbit/dynamic-snippet branch August 28, 2023 19:51
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.

Update snippet based on selection
3 participants