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 Button entry #198

Merged
merged 3 commits into from Mar 5, 2023
Merged

Conversation

SoNiC-HeRE
Copy link
Contributor

Created Button Demo under User Interface Section in Library
Issue workbenchdev/demos#3

@sonnyp sonnyp requested review from sonnyp and bertob February 25, 2023 17:29
Copy link

@bertob bertob left a comment

Choose a reason for hiding this comment

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

So in general this should split out the different types of buttons a bit more, it's mixing things too much. I'd not make them all pill buttons, and have separate buttons for showing the main types of buttons. I'd try a grid with normal, suggested, destructive, etc. similar to what's in the style classes demo in libadwaita.

I'd also not show .flat buttons here, because they're a special thing mainly to be used in constrained environments like toolbars/headerbars.

image


Adw.StatusPage{
title: "Button";
description: "A simple way to trigger an event";
Copy link

Choose a reason for hiding this comment

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

"Allow people to perform actions by clicking"

{
"name": "Button",
"category": "user_interface",
"description": "A simple way to trigger an event",
Copy link

Choose a reason for hiding this comment

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

Same as above

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.

Agree with Tobias

There should be

  • Regular
  • Suggested
  • Destrucive
  • Custom (1)
  • Pill
  • Circular
  • osd
  • Disabled

The challenge is to make the grid look good.

You can take inspiration from libadwaita demo:

image

if you hover one of them - it tells you what CSS class is used

Comment on lines 40 to 42
<file>Library/demos/Button/main.blp</file>
<file>Library/demos/Button/main.json</file>
<file>Library/demos/Button/main.js</file>
Copy link
Contributor

Choose a reason for hiding this comment

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

this is not needed anymore (you will have to merge main into your branch)

#200

Comment on lines 42 to 43

}
Copy link
Contributor

Choose a reason for hiding this comment

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

please take care of the indentation until it's automated

it should be 2 spaces

dialog.present();
}

test_button.connect("clicked", handle_click);
Copy link
Contributor

Choose a reason for hiding this comment

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

the dialog is a bit annoying while testing because the user needs to close the modal to proceed - it is 'blocking'

Also Gtk.MessageDialog will be deprecated in GTK 4.10

I don't have a much better idea - I'd say console.log("Hello, World!"); is enough for now

@SoNiC-HeRE
Copy link
Contributor Author

Okay understood. I'll make the necessary changes.

@sonnyp sonnyp changed the title Added Button Demo library: add Button entry Feb 27, 2023
@sonnyp sonnyp changed the title library: add Button entry library: Add Button entry Feb 28, 2023
@SoNiC-HeRE
Copy link
Contributor Author

Further to enhance the design i intend to smaller the icon-buttons respective to the class buttons but somehow due to the grid the buttons scale equally.

@SoNiC-HeRE SoNiC-HeRE requested a review from sonnyp March 1, 2023 13:54
@SoNiC-HeRE
Copy link
Contributor Author

Applied Changes to match styling as of Adwaita Demo

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.

Thanks!

I did some cleanups before merging 8f5fc6e

FlowBox didn't do anything useful and I simplified a bit the JS code

@sonnyp sonnyp merged commit d457a64 into workbenchdev:main Mar 5, 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