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

Fix make checkbox text clickable in FileDialog #95304

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Giganzo
Copy link
Contributor

@Giganzo Giganzo commented Aug 9, 2024

Fixes #95079

Copy link
Member

@Calinou Calinou left a comment

Choose a reason for hiding this comment

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

Tested locally, it works as expected.

image

@Calinou Calinou modified the milestones: 4.3, 4.4 Aug 9, 2024
@Calinou Calinou added cherrypick:4.3 Considered for cherry-picking into a future 4.3.x release topic:gui and removed topic:editor topic:export labels Aug 9, 2024
@akien-mga akien-mga requested a review from bruvzg August 12, 2024 15:31
@akien-mga
Copy link
Member

@bruvzg I see you've another draft as #95140 doing this slightly differently.
In order to (possibly) fix the regression for 4.3, do you think this approach is fine as a quick fix?

Otherwise the regression isn't critical so it's fine to have it wait.

Copy link
Member

@bruvzg bruvzg left a comment

Choose a reason for hiding this comment

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

This will immediately break for any file dialog with more than one option, it's using Grid container and should have the same number of child controls in every row.

Screenshot 2024-08-12 at 18 45 25

@bruvzg
Copy link
Member

bruvzg commented Aug 12, 2024

It's not critical and I'n not sure what's the best visual way to do it (#95140 probably is not the best), so I would not do anything for 4.3.

@Giganzo Giganzo force-pushed the fix-filedialog-checkbox-text-clickable branch from e520a13 to 22f2337 Compare September 21, 2024 18:13
@Giganzo
Copy link
Contributor Author

Giganzo commented Sep 21, 2024

The idea was that this could be used as a quick fix. Not sure if it still needed or if we should wait for some other solution.

@KoBeWi
Copy link
Member

KoBeWi commented Sep 21, 2024

tbh the initial solution could be fine if you added an empty Control to take Label's space.

@Giganzo
Copy link
Contributor Author

Giganzo commented Sep 21, 2024

How it looks now with vbox.
Screenshot_20240921_160811

tbh the initial solution could be fine if you added an empty Control to take Label's space.

With GridContainer and extra Control it looks like this.
Screenshot_20240921_162419

Can revert to the GridContainer if you think it's better and this pr is still needed.

@KoBeWi
Copy link
Member

KoBeWi commented Oct 15, 2024

The latter looks better, because the controls are better aligned.
However there is #97892, which might supersede this PR (it was opened independently and just happens to fix the same issue).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug cherrypick:4.3 Considered for cherry-picking into a future 4.3.x release regression topic:gui usability
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Export With Debug text can't be clicked
5 participants