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

Modal dialog improvement #1187

Merged
merged 7 commits into from
Mar 10, 2024
Merged

Modal dialog improvement #1187

merged 7 commits into from
Mar 10, 2024

Conversation

navv-1
Copy link
Contributor

@navv-1 navv-1 commented Mar 9, 2024

This pr will fix this issue,
image

and fix #1176

@justvanrossum
Copy link
Collaborator

This must be related to #1175 and https://github.com/googlefonts/fontra/pull/1180/files, and I had an uneasy feeling about the fix.

I have an uneasy feeling about the fix in this PR, too: I don't think max-height: 32em; is what we want: it should be able to get as big as possible, with some space above and below.

Indeed, I just tested: try "Add Component", this should show a big list, and it should be as big as possible (within roughly the 80vh).

@navv-1
Copy link
Contributor Author

navv-1 commented Mar 9, 2024

Actually, we can also remove the max-height and let it overflow since we have the scrollbar on the edge
image

@justvanrossum
Copy link
Collaborator

Hmm, we shouldn't have that scrollbar at the edge. The dialog should fit on the page (with some margin above and below), and any scrolling should be handled by its contents.

Your original problem is that we get too many scrollbars when the window isn't tall enough, right?

@justvanrossum
Copy link
Collaborator

Can you compare how the situation was before #1146? I feel that PR had larger consequences than intended.

@navv-1
Copy link
Contributor Author

navv-1 commented Mar 9, 2024

Look at the add and remove button, if we use vh we will get this result when the window isn't tall enough
image

@justvanrossum
Copy link
Collaborator

So what I'm asking is, did that issue exist before #1146 was merged?

@navv-1
Copy link
Contributor Author

navv-1 commented Mar 9, 2024

this is after #1145 merged
image
image

@justvanrossum
Copy link
Collaborator

Ok, so we need to back up more. Was it ok before #1145? Was it ever ok? I need to know whether this is a regression or something that was always broken.

@navv-1
Copy link
Contributor Author

navv-1 commented Mar 9, 2024

I tried after #929 merged and it's not different

@justvanrossum
Copy link
Collaborator

And before that one?

@navv-1
Copy link
Contributor Author

navv-1 commented Mar 9, 2024

Yes, even before the menu bar was merged. I think it's not regression.

@navv-1
Copy link
Contributor Author

navv-1 commented Mar 9, 2024

Indeed, I just tested: try "Add Component", this should show a big list, and it should be as big as possible (within roughly the 80vh).

Instead of looking at a long list, I prefer to use the search bar and get a shorter list.

@justvanrossum
Copy link
Collaborator

justvanrossum commented Mar 9, 2024

Instead of looking at a long list, I prefer to use the search bar and get a shorter list.

I disagree. I don't want a hardcoded maximum height for the dialog.

I feel that e98e75c / #1170 was wrong, and we should fix #1168 in a different way.

@navv-1
Copy link
Contributor Author

navv-1 commented Mar 9, 2024

I just brought back the max-height. The only problem that I found is in the add and remove button. But this kind of situation maybe would never happen in the real world :D
image

@justvanrossum
Copy link
Collaborator

Thanks!

That might be related to the minHeight of the axis list:

@justvanrossum
Copy link
Collaborator

Changing that to "3em" seems to improve it.

@navv-1
Copy link
Contributor Author

navv-1 commented Mar 10, 2024

I changed that to 10vh and I think it's much better.

image

I also add min-height: 10em to the dialog box to prevent it from shrinking too short. It makes dialog a bit higher when it has a short message. I hope it's still acceptable.

image

@justvanrossum
Copy link
Collaborator

Hm, I'm not happy with the use of vh in the list minHeight. This should be an absolute value, and should not depend on the view height. You can tweak the exact value of 3em, but please use em.

Secondly, I am not happy with the taller dialog for short messages. If this is to make sure the dialog doesn't break for a very small window height: I'm actually ok with some breakage under such tight conditions. The app is not really meant for such small windows.

@navv-1
Copy link
Contributor Author

navv-1 commented Mar 10, 2024

Ok, I just follow your suggestion

@justvanrossum justvanrossum merged commit bb3a255 into googlefonts:main Mar 10, 2024
3 checks passed
@justvanrossum
Copy link
Collaborator

Thank you once again for your contributions! It's much appreciated.

@navv-1 navv-1 deleted the modal-dialog branch March 11, 2024 03:04
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.

Modal dialog shows scroll bar
2 participants