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

Adding selection option to Label #5536

Open
wants to merge 21 commits into
base: develop
Choose a base branch
from

Conversation

andydotxyz
Copy link
Member

@andydotxyz andydotxyz commented Feb 16, 2025

Selectable label incoming.

Got some tests to write/fix and a little refactoring to do - but I am pretty sure it's working well and has the right public API.

Checklist:

  • Tests included.
  • Lint and formatter run with no errors.
  • Tests all pass.
  • Public APIs match existing style and have Since: line.

@andydotxyz andydotxyz force-pushed the feature/selectablelabel branch 2 times, most recently from bebc4e6 to f24aedc Compare February 16, 2025 15:54
@andydotxyz andydotxyz force-pushed the feature/selectablelabel branch from f24aedc to 40f9284 Compare February 16, 2025 15:55
@coveralls
Copy link

coveralls commented Feb 21, 2025

Coverage Status

coverage: 62.412% (+0.1%) from 62.294%
when pulling cab213e on andydotxyz:feature/selectablelabel
into f642bd5 on fyne-io:develop.

@andydotxyz andydotxyz marked this pull request as ready for review February 24, 2025 16:22
Copy link
Member

@Jacalz Jacalz left a comment

Choose a reason for hiding this comment

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

Thanks. It is great to finally see this much requested feature. Well done. I just added some minor comments inline.

@andydotxyz andydotxyz requested a review from Jacalz February 25, 2025 15:27
Copy link
Member

@Jacalz Jacalz left a comment

Choose a reason for hiding this comment

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

Hmm. I was testing this some more now before intending to approve and it seems like clicking outside of the label does not remove the selection?

@andydotxyz
Copy link
Member Author

Hmm. I was testing this some more now before intending to approve and it seems like clicking outside of the label does not remove the selection?

Thanks, all fixed plus I found some issues with mobile details as well, working well now I think.

@andydotxyz
Copy link
Member Author

Gah, missed the change in one file. Could have force pushed but forgot. Ah well, should be green now.

Jacalz
Jacalz previously approved these changes Feb 26, 2025
Copy link
Member

@Jacalz Jacalz left a comment

Choose a reason for hiding this comment

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

Tests seem to be failing but otherwise approved 👍

@andydotxyz andydotxyz requested a review from dweymouth February 26, 2025 18:31
widget/entry.go Outdated
@@ -288,6 +273,8 @@ func (e *Entry) FocusGained() {
e.setFieldsAndRefresh(func() {
e.dirty = true
e.focused = true

log.Println("FOCUS")
Copy link
Contributor

Choose a reason for hiding this comment

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

Found a left-over.

Copy link
Member Author

Choose a reason for hiding this comment

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

How embarrassing! Fixed

Copy link
Member

Choose a reason for hiding this comment

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

The import was accidentally left in leading to a compile error unfortunately

Copy link
Member Author

Choose a reason for hiding this comment

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

Apologies, I should stop doing code commits on the bus!

Copy link
Member

Choose a reason for hiding this comment

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

Relatable. No problem

Copy link
Contributor

Choose a reason for hiding this comment

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

Different kind of bus factor, and we're all in for the ride 😉

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.

5 participants