-
-
Notifications
You must be signed in to change notification settings - Fork 290
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
MBS-1964: Open "Add selected X for merging" in a new tab #3474
base: master
Are you sure you want to change the base?
Conversation
This seems like the best option in order to allow editors to work through the lists without having to keep going back to them. This was supposed to be achieved by the current returnToCurrentPage, but that seems broken. In any case, several users seem to prefer leaving the previous page untouched (with checkboxes selected, etc) while opening the merge form in a new tab.
It's no big deal, but if this is outside the usual MB behaviour (do we ever open anything else in a new tab?), it might be worth adding a little icon to the button to indicate that it will open in a new tab: https://thenounproject.com/browse/icons/term/new-tab/ I can open another ticket with a mockup etc if this sounds useful :) p.s. thank you for addressing this so fast! |
I am surprised that a good old form with
I first thought this was sarcasm because the ticket number is very low, before I realized that this PR was probably created as a reaction to your comment on the ticket, within a few hours 😅 |
We open lots of things in new tabs, but it's mostly about docs and things like that. I have no strong opinion about whether an icon can or cannot help, or enough knowledge to know if anything is needed for accessibility purposes. This is failing tests because our old validator doesn't accept these attributes in forms. If we update the validator, it (correctly) rejects some other crap added by react-table, we'll need to update and fix that, but we'll get there. |
An icon when opening in a new tab/window is certainly preferred, but I gather from your answer that it could be a headache to implement? I don't want to slow down yet another PR. In any case, let me know if I can provide or help with anything. |
It could be a headache to implement everywhere, it shouldn't be hard to implement here for now. I was just wondering if that is enough or if something else is needed because normally "icon with no text" is seen as a bad thing from an accessibility perspective. But I guess at least with screen readers and whatnot there must be a way to read the I also hate those extra icons and I'd prefer hiding them for myself, but I still can see how they are useful for the weirdos who like opening things in the same tab 😝 |
To be honest I had no idea that anyone didn't like those icons! I just thought they are unobtrusive and a universally appreciated indication of what's going to happen (instead of surprising the user). I don't think any extra text is needed - alt text for a icon/image is always a requirement though imo. Anyway, if the prospect doesn't excite you, don't worry about it! I made a separate ticket: https://tickets.metabrainz.org/browse/MBS-13941 |
👎 This PR is removing the choice between same tab (click) and new tab (shift+click) from the user. Now, it will always be new tab, and no way to choose same tab. Sometimes I click, sometimes I shift+click. IMO, it's anti-QOL. ;) And those who don't master MB behaviour may open multiple tabs now, without knowing all merges will cumulate in the last tab or, worse, each new tab will cancel the earlier tabs, if it's a different entity type. |
Implement MBS-1964
Description
It would be nice to be able to open the merge form from "Add selected X for merging" in entity lists (such as
artist/mbid/recordings
) in a new tab. This seems like the best option in order to allow editors to work through the lists without having to keep going back to them.This was supposed to be achieved by the current
returnToCurrentPage
option, but that seems broken. In any case, several users seem to prefer leaving the previous page untouched (with checkboxes selected, etc) while opening the merge form in a new tab.As such, this adds
target="_blank"
to the relevantform
s, which seems supported and working (and in fact is what a userscript already does).Testing
Manually.
Further actions
We should remove the broken
returnToCurrentPage
bits (added by @mwiencek in 69df512) unless we fix it, but some users have said they prefer the merge to actually show the destination entity and IMO if we're going to have it in a new tab that makes more sense than going back to the page we still have open anyway.