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.

Sign up for GitHub

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

Jump to bottom

library: Add Text Fields entry #205

Merged
merged 9 commits into from
Mar 14, 2023
Merged

library: Add Text Fields entry #205

merged 9 commits into from
Mar 14, 2023

Conversation

AkshayWarrier
Copy link
Contributor

Adds Text Input ( Entry / PasswordEntry / SearchEntry ) widgets.

Issue workbenchdev/demos#3

sonnyp
sonnyp reviewed Mar 7, 2023
View reviewed changes
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.

Very nice, this must be the most complex entry so far.

Could you add a new section at the bottom "Style Classes" with something similar to what is in libadwaita demo? No need for regular again.

image

src/Library/demos/Text Input/main.blp Outdated Show resolved Hide resolved
src/Library/demos/Text Input/main.blp Outdated Show resolved Hide resolved
src/Library/demos/Text Input/main.blp Outdated Show resolved Hide resolved
src/Library/demos/Text Input/main.blp Outdated Show resolved Hide resolved
src/Library/demos/Text Input/main.blp Outdated Show resolved Hide resolved
src/Library/demos/Text Input/main.blp Outdated
margin-bottom: 10;
}
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice adding the search entries/results 👍

However — it's not really how GNOME would do it because it makes content reflow. Consider using a scrolled window.

WDYT of using GtkEntryCompleton instead? There is a demo in GTK Demo
I know it's deprecated but GTK5 will take years and we can port to whatever new mechanism is added then.
I think it will make the code simpler too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For the content reflow, do you want me to wrap the ListBox in a ScrolledWindow?

I thought of using GtkEntryCompletion but since it was deprecated I didn't think to show it. If you're fine with it, I can replace the Search Entry with a GtkEntryCompletion demo instead.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, let's make an exception for this one and use the deprecated API since there is no replacement just yet. It was deprecated to get rid of cell renderer (the underlying backend) — not because we don't have to have this UI pattern.

I'm sure we'll get a replacement.

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

sonnyp commented Mar 7, 2023

one more thing - don't forget yo add yourself to contributors in about.js :)

@AkshayWarrier
Copy link
Contributor Author

Okay I will :)

@AkshayWarrier AkshayWarrier changed the title library: Add Text Input entry Draft: library: Add Text Fields entry Mar 9, 2023
@AkshayWarrier AkshayWarrier changed the title Draft: library: Add Text Fields entry Draft:library: Add Text Fields entry Mar 9, 2023
@AkshayWarrier AkshayWarrier changed the title Draft:library: Add Text Fields entry Draft: library: Add Text Fields entry Mar 9, 2023
@AkshayWarrier AkshayWarrier changed the title Draft: library: Add Text Fields entry ibrary: Add Text Fields entry Mar 9, 2023
@AkshayWarrier AkshayWarrier changed the title ibrary: Add Text Fields entry library: Add Text Fields entry Mar 9, 2023
@AkshayWarrier AkshayWarrier marked this pull request as draft March 9, 2023 08:30
@AkshayWarrier
Copy link
Contributor Author

There we go okay.

This PR has been marked as draft because of an issue with GtkEntryCompletion not working properly. But I have made the other changes like you have asked and are ready to be reviewed.

@sonnyp sonnyp self-assigned this Mar 11, 2023
sonnyp
sonnyp reviewed Mar 13, 2023
View reviewed changes
Copy link
Contributor

@sonnyp sonnyp left a comment •
edited
Loading

Choose a reason for hiding this comment

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

see #205 (comment)

@AkshayWarrier AkshayWarrier marked this pull request as ready for review March 13, 2023 04:28
@AkshayWarrier
Copy link
Contributor Author

Is there anything else left to be done for this PR then?

@AkshayWarrier
Copy link
Contributor Author

I do get this error when loading the demo

(re.sonny.Workbench.Devel:4): Gjs-WARNING **: 16:14:56.772: Some code tried to set a deprecated GObject property.
0 <TOP LEVEL> ["file:///tmp/workbench-686C11.js":58:0]
1 InnerModuleEvaluation() ["self-hosted":2355:30]
2 evaluation() ["self-hosted":2312:23]

But I guess it can't be helped.

@sonnyp sonnyp merged commit d309ff7 into workbenchdev:main Mar 14, 2023
@AkshayWarrier AkshayWarrier deleted the add-text-input-entry branch March 15, 2023 03:19
sonnyp pushed a commit to SoNiC-HeRE/Workbench that referenced this pull request Mar 21, 2023
@AkshayWarrier @sonnyp
library: Add Text Fields entry (workbenchdev#205)
22234f9
andyholmes pushed a commit to andyholmes/Workbench that referenced this pull request Mar 26, 2023
@AkshayWarrier @andyholmes
library: Add Text Fields entry (workbenchdev#205)
7f953e5
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sonnyp sonnyp sonnyp approved these changes

Assignees

@sonnyp sonnyp

Labels
Library 📚️
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@AkshayWarrier @sonnyp