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

library: Add Box entry #216

Merged
merged 8 commits into from
Mar 13, 2023
Merged

Conversation

AkshayWarrier
Copy link
Contributor

This PR attempts to add an interactive demo for Gtk.Box

Issue workbenchdev/demos#3

Copy link
Contributor

@andyholmes andyholmes left a comment

Choose a reason for hiding this comment

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

This looks good and I especially like the UX of the slider to change the alignment.

src/Library/demos/Box/main.blp Outdated Show resolved Hide resolved
src/Library/demos/Box/main.blp Outdated Show resolved Hide resolved
src/Library/demos/Box/main.blp Outdated Show resolved Hide resolved
src/Library/demos/Box/main.css Outdated Show resolved Hide resolved
src/Library/demos/Box/main.css Show resolved Hide resolved
src/Library/demos/Box/main.css Outdated Show resolved Hide resolved
src/Library/demos/Box/main.js Outdated Show resolved Hide resolved
src/Library/demos/Box/main.js Outdated Show resolved Hide resolved
Copy link
Contributor

@sonnyp sonnyp left a comment

Choose a reason for hiding this comment

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

Awesome to get an interactive Box demo 👍

src/Library/demos/Box/main.blp Outdated Show resolved Hide resolved
src/Library/demos/Box/main.blp Outdated Show resolved Hide resolved
src/Library/demos/Box/main.blp Outdated Show resolved Hide resolved
Comment on lines 58 to 60
CheckButton toggle_orient {
label: "Toggle Orientation";
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Please use the the name of the property and the name of the possible values so it clears to users what we are changing here.

orientation: vertical|horizontal

I'd suggest using grouped toggle buttons (see #219)

src/Library/demos/Box/main.blp Outdated Show resolved Hide resolved
src/Library/demos/Box/main.blp Outdated Show resolved Hide resolved
src/Library/demos/Box/main.blp Outdated Show resolved Hide resolved
@AkshayWarrier AkshayWarrier marked this pull request as draft March 12, 2023 15:39
@AkshayWarrier AkshayWarrier marked this pull request as ready for review March 12, 2023 22:42
@sonnyp sonnyp requested a review from andyholmes March 12, 2023 23:34
Copy link
Contributor

@andyholmes andyholmes left a comment

Choose a reason for hiding this comment

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

This really great for demonstrating both GtkBox and alignment/expansion, well done!

Just a few layout tweaks and I think this is good to go if @sonnyp agrees.

src/Library/demos/Box/main.blp Show resolved Hide resolved
src/Library/demos/Box/main.blp Show resolved Hide resolved
src/Library/demos/Box/main.blp Show resolved Hide resolved
Copy link
Contributor

@andyholmes andyholmes left a comment

Choose a reason for hiding this comment

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

Great job!

Copy link
Contributor

@sonnyp sonnyp left a comment

Choose a reason for hiding this comment

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

Fantastic 👍

I added a couple of tooltips to clarify what "halign" / "valign" stands for

6c984a1

@sonnyp sonnyp merged commit 00be084 into workbenchdev:main Mar 13, 2023
@AkshayWarrier
Copy link
Contributor Author

Thanks! 😄

@AkshayWarrier AkshayWarrier deleted the add-box-entry branch March 15, 2023 03:19
sonnyp pushed a commit to SoNiC-HeRE/Workbench that referenced this pull request Mar 21, 2023
andyholmes pushed a commit to andyholmes/Workbench that referenced this pull request Mar 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants