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

Sync code snippet with selected icon #29

Closed
wants to merge 11 commits into from

Conversation

Antolius
Copy link
Member

@Antolius Antolius commented Nov 23, 2022

Resolves #12

I wrapped icons in toggle buttons, tracked the selected one and updated example code snippet accordingly. This also fixes an issue with incorrect snippet for symbolic only icons or those without size 24px, for which the static snippet doesn't work.

Example of using it:

dynamic-snippet-showcase.webm

Wrap icons in toggle buttons, track the selected one and
update example code snippet accordingly. This also fixes
an issue with incorrect snippet for symbolic only icons
or those without size 24px.
@Antolius
Copy link
Member Author

Sorry for the linting errors, I'll fix them tonight.

Add missing white-space and convert snippet template
into a multi-line string.
@lenemter lenemter requested a review from danirabbit December 10, 2022 11:55
@zeebok
Copy link

zeebok commented Feb 4, 2023

The code itself seems fine, but I do wonder about the styling. Is there a way to easily have the buttons be a uniform width? For example when there is only one icon in a group, it taking up the full width doesn't feel nice, to me anyway. Maybe someone from @elementary/ux can weigh in.

Limit size of toggle buttons for selecting icons by wrapping them inside a Adw.Clamp widget with maximum size of 128. Introduce dependency on libadwaita-1, which can be removed after Granite issue #623 is resolved.
@Antolius
Copy link
Member Author

Antolius commented Feb 7, 2023

Thanks for the feedback @zeebok! Indeed, horizontally expanded buttons look bad.

I tried playing around with them a bit, and I think I made some progress. I added a size constraint using the Adw.Clamp. I had to introduce a dependency on LibAdwaita for that, but we will be able to remove it once Granite issue #623 is resolved. Here are a few examples of the new look:

Screenshot from 2023-02-07 18 29 14
Screenshot from 2023-02-07 18 30 12

This is what it looks like in full screen layout, when the app has mode horizontal space to work with:

Screenshot from 2023-02-07 18 30 07
Screenshot from 2023-02-07 18 30 05

@zeebok
Copy link

zeebok commented Feb 20, 2023

This is definitely better and am okay with it. I think before any final thumbs-up for merging it should get the ok from @elementary/ux or at least @danirabbit for the styling since she had the original vision for the app.

Copy link
Member

@danirabbit danirabbit left a comment

Choose a reason for hiding this comment

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

Probably what would be a cleaner way to do this is use a Gtk.FlowBox. This would give us a lot of things for free like setting a single selection mode, the ability to make sure children are homogeneous, and signals for when a child is selected

@danirabbit
Copy link
Member

@Antolius Can you give a look to #32?

@Antolius
Copy link
Member Author

Antolius commented May 7, 2023

Ah, yes, using Gtk.FlowBox makes much more sense than the shenanigans I was up to here. I'll close this PR in favor of #32

@Antolius Antolius closed this May 7, 2023
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