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

Add reference-font remove button #2017

Merged

Conversation

giovanniTramonto
Copy link
Contributor

Before reference-font items could only be deleted by selecting (clicking) an item and pressing delete-key after. Not self-explanatory.
Now there is a remove (-) button at the end of each line.

Bildschirmfoto 2025-02-10 um 16 01 28

@giovanniTramonto giovanniTramonto force-pushed the add-reference-font-remove-button branch from 04a7038 to 9f9a837 Compare February 10, 2025 19:35
@justvanrossum
Copy link
Collaborator

justvanrossum commented Feb 11, 2025

We use at least two different things for this in Fontra:

  • (+) (-) buttons below the list (for example in glyph source list)
  • a "trash" icon at the right end side of a panel/row/thing

In the context of lists, so far we only use the (+) (-) approach. So that would be more consistent with other parts of Fontra. (The "+" button could trigger a file upload dialog)

On the other hand, to have a delete icon ("trash" would be preferred then), is a bit more user friendly, as it doesn't require an item to be selected first.

I don't think this icon needs to be so deeply integrated into the ui-list class. Look at for example the glyph source list: it contains clickable icons.

I'm leaning towards to (+) (-) solution.

@giovanniTramonto
Copy link
Contributor Author

Ah yes, I see, that’s much easier, actually. Thanks for the hint

@giovanniTramonto
Copy link
Contributor Author

giovanniTramonto commented Feb 11, 2025

I reversed changes in ui-list.js
So after staching commits there wont be changes in ui-list.

@ollimeier ollimeier self-requested a review February 12, 2025 09:02
Copy link
Collaborator

@ollimeier ollimeier left a comment

Choose a reason for hiding this comment

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

Hi @giovanniTramonto (Johannes), thanks a lot that you help us with Fontra. I much appreciate that and it's great to be in contact with you this way, again.
I tested it and it works well. Please see my comments.

@giovanniTramonto
Copy link
Contributor Author

Hi @ollimeier!
Thanks for the review. Yes, indeed, great to work with you guys on this. Appreciate it very much, too.
I have implemented all suggestions as well as some improvement on the selection: Now, it automatically selects last row after removal, so it’s easier to remove multiple items as well.
I spot that the supported languages do not update when the list becomes empty. If it’s OK with you I would adjust this in a follow-up.

Copy link
Collaborator

@ollimeier ollimeier left a comment

Choose a reason for hiding this comment

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

Thanks a lot for your update. Seems to work well. I have only a few minor change requests related to the text strings in the dialog.
I noticed that the reference font is not displayed if we add only one font. Here a quick demo:

Screen.Recording.2025-02-13.at.09.06.43.mp4

This was not introduced by you. It was there already before. We may want to have a look into that issue as a follow up.

@giovanniTramonto
Copy link
Contributor Author

Thanks a lot for your update. Seems to work well. I have only a few minor change requests related to the text strings in the dialog. I noticed that the reference font is not displayed if we add only one font. Here a quick demo:

Screen.Recording.2025-02-13.at.09.06.43.mp4
This was not introduced by you. It was there already before. We may want to have a look into that issue as a follow up.

Oh, interesting! Just tried to reproduce. For me it seems to work:

reference-font.mov

@justvanrossum
Copy link
Collaborator

Is it possible for the "+" button to immediately do to the choose files dialog, without the in-between dialog?

@giovanniTramonto
Copy link
Contributor Author

Oh, yes. This is much more practical

@ollimeier
Copy link
Collaborator

@justvanrossum @giovanniTramonto This last change is so much better. Keep it simple, right!
I tested it and it works well. I am going to give it an approval, but I will not merge it. So, that you Just have the chance to look at it.

Copy link
Collaborator

@justvanrossum justvanrossum left a comment

Choose a reason for hiding this comment

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

I left one comment, otherwise looks good to me.

@justvanrossum justvanrossum dismissed ollimeier’s stale review February 14, 2025 17:28

I think this review has been resolved, I'm not sure why it isn't marked as such.

Copy link
Collaborator

@justvanrossum justvanrossum left a comment

Choose a reason for hiding this comment

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

Thanks.

Should the remove button perhaps also respond to the alt key, to mean "delete all"?

So currently no item gets selected after removing the selection, right? This matches the original behavior, but not wat we recently discussed. Is that your intention indeed?

@giovanniTramonto
Copy link
Contributor Author

Thanks.

Should the remove button perhaps also respond to the alt key, to mean "delete all"?

So currently no item gets selected after removing the selection, right? This matches the original behavior, but not wat we recently discussed. Is that your intention indeed?

Now, it selects the item before in the list, or after in case its the first list item. Feels good

@giovanniTramonto
Copy link
Contributor Author

giovanniTramonto commented Feb 17, 2025

For the deleting multiple files, I guess, really nice would be to select a range with shift key.

But I have to solve this render glitch before:

glitch.mov

@justvanrossum
Copy link
Collaborator

For the deleting multiple files, I guess, really nice would be to select a range with shift key.

Yes, but the UIList class currently does not support that. While we need that eventually, it is out of scope for this PR.

But I have to solve this render glitch before:

Is it related to the file names being wrapped?

@justvanrossum
Copy link
Collaborator

I'm willing to merge this as it is: it works well and the code looks fine to me. What do you say?

giovanniTramonto added a commit to giovanniTramonto/fontra that referenced this pull request Feb 18, 2025
@giovanniTramonto
Copy link
Contributor Author

Glitch is fixed. It was related to the render update of ui-list which caused update of parent grid. Moving the list inside a wrapper helped. I also set keepScrollPosition to true, so when having many items after removal user can still see the selected item without scrolling down again. If that’s all fine with you, I’m willing to merge, too.

@justvanrossum
Copy link
Collaborator

But now the layout behavior is different: the preview section at the bottom takes up more space than it needs when the size is small, leaving a lot of space unused:

image

@giovanniTramonto giovanniTramonto force-pushed the add-reference-font-remove-button branch from abc2e2e to 566498c Compare February 19, 2025 16:43
@giovanniTramonto
Copy link
Contributor Author

OK, I reset the layout changes and only set the keep-scroll-position, which I guess is useful when deleting items. I compared with main which has the glitch, too. So this MR does not cause it and I can look at this in a follow-up. Feel free to merge.

@justvanrossum justvanrossum merged commit ed2fb85 into googlefonts:main Feb 24, 2025
5 checks passed
@giovanniTramonto giovanniTramonto deleted the add-reference-font-remove-button branch February 24, 2025 20:46
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.

3 participants