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 Frame Entry #317

Merged
merged 10 commits into from Jun 22, 2023
Merged

library: Add Frame Entry #317

merged 10 commits into from Jun 22, 2023

Conversation

SoNiC-HeRE
Copy link
Contributor

Currently implemented a simple UI to demonstrate usage of Gtk.Frame but can be implemented in a more interactive way that shows more complex use of Gtk.Frame such as:

  • Collapsible Frame Sections:
    Divide the frame's content into collapsible sections using Gtk.Expander widgets. Each section can be expanded or collapsed individually, allowing users to focus on specific parts of the frame's content.
  • Frame with Toolbar:
    Embed a Gtk.Toolbar within the frame to provide additional functionality or actions related to the frame's contents. The toolbar can contain buttons, icons, or dropdown menus to perform specific tasks.
  • Or just for enhancing UI

Closes #316

Open to suggestions and looking for an early review

@SoNiC-HeRE SoNiC-HeRE marked this pull request as draft June 11, 2023 13:00
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.

I like the idea of toolbars and revealers. It might be interesting to contrast this demo with the TextView demo where you put it in a scrollable frame.

For example, in the TextView demo everything will fit on "one page" and you can scroll in the area. But other situations you want the page to scroll, which is usually what you want for fixed content like labels and images.

One thing I'd try to avoid is duplicating the use-case of Adw.ExpanderRow too closely, because it might add some confusion about which one to use.

Side-note: Gtk.Toolbar doesn't exist in GTK4, because it was basically just a Gtk.Box with some special styling. So we just use Gtk.Box (or Gtk.CenterBox) with the toolbar CSS class.

@SoNiC-HeRE
Copy link
Contributor Author

I like the idea of toolbars and revealers. It might be interesting to contrast this demo with the TextView demo where you put it in a scrollable frame.

For example, in the TextView demo everything will fit on "one page" and you can scroll in the area. But other situations you want the page to scroll, which is usually what you want for fixed content like labels and images.

One thing I'd try to avoid is duplicating the use-case of Adw.ExpanderRow too closely, because it might add some confusion about which one to use.

Side-note: Gtk.Toolbar doesn't exist in GTK4, because it was basically just a Gtk.Box with some special styling. So we just use Gtk.Box (or Gtk.CenterBox) with the toolbar CSS class.

How about demonstrating different usecases in a scrollable view?

  1. Layout only
  2. Toolbar
  3. TextView

wdyt?

@andyholmes
Copy link
Contributor

Yeah, that sounds good. Let's give that a try and see how it looks.

@SoNiC-HeRE
Copy link
Contributor Author

SoNiC-HeRE commented Jun 16, 2023

Made the following changes:

  • Divided the examples in 3 sections with different usecases
  • Implemented a clean UI
  • Added necessary JS Code

P.S: Thought of adding an example with Gtk.Label but did'nt go well with the UI.

@SoNiC-HeRE SoNiC-HeRE marked this pull request as ready for review June 16, 2023 14:39
@sonnyp sonnyp changed the title library: Adds Frame Entry library: Add Frame Entry Jun 17, 2023
@sonnyp sonnyp removed the request for review from andyholmes June 17, 2023 20:21
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.

Looks great.

Only issue is we need to use an image in public domain.

What about this one https://www.publicdomainpictures.net/en/view-image.php?image=508689&picture=vintage-illustration-cat-kitten ?

src/Library/demos/Frame/main.blp Outdated Show resolved Hide resolved
@SoNiC-HeRE
Copy link
Contributor Author

Looks great.

Only issue is we need to use an image in public domain.

What about this one https://www.publicdomainpictures.net/en/view-image.php?image=508689&picture=vintage-illustration-cat-kitten ?

works

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.

Looks good, just a few tweaks and it's good to go!

src/Library/demos/Frame/main.blp Outdated Show resolved Hide resolved
src/Library/demos/Frame/main.blp Show resolved Hide resolved
src/Library/demos/Frame/main.blp Outdated Show resolved Hide resolved
@sonnyp
Copy link
Contributor

sonnyp commented Jun 18, 2023

@SoNiC-HeRE please update the image #317 (review)

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.

Nice one!

@andyholmes andyholmes requested a review from sonnyp June 20, 2023 17:15
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.

One last thing

the label needs to be center with the widget on top in the first and last examples

image

@SoNiC-HeRE SoNiC-HeRE requested a review from sonnyp June 21, 2023 09:53
@SoNiC-HeRE
Copy link
Contributor Author

SoNiC-HeRE commented Jun 21, 2023

Fixed Spacing ,removed arbitrary margins and used box properties

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.

Labels are still not centered vertically with the widget they describe

image

homogeneous: true;
margin-bottom: 24;
halign: center;
spacing: 108;
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 also considered an arbitrary number

please use box layout for centering
ask if you need help

@sonnyp
Copy link
Contributor

sonnyp commented Jun 22, 2023

Please be more thorough

@SoNiC-HeRE
Copy link
Contributor Author

SoNiC-HeRE commented Jun 22, 2023

Please be more thorough

Yes, i think i misunderstood something ; fixed the error
Updated with Box layout

I think the label of textview without frame is slightly above when compared to label with frame but I did'nt change the box spacing from 12 to 18 neither bottom-margin of textview without frame as it would create a significant bigger gap

@SoNiC-HeRE SoNiC-HeRE requested a review from sonnyp June 22, 2023 13:02
@sonnyp sonnyp merged commit 0158d75 into workbenchdev:main Jun 22, 2023
sonnyp pushed a commit that referenced this pull request Jul 5, 2023
sonnyp pushed a commit to SoNiC-HeRE/Workbench that referenced this pull request Aug 13, 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.

Frame Entry
3 participants