-
-
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: Allow opening "Add selected X for merging" in a new tab #3474
base: master
Are you sure you want to change the base?
Conversation
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. |
8099425
to
1a86ab3
Compare
This seems like the best option in order to allow editors to work through the lists without having to keep going back to them. Some editors wanted to keep the current option for some reason so this adds a second button so that the editor has the choice. In both cases, the checkboxes are cleared when submitting the merge, allowing to easily continue using the page to merge further entries if desired. This was supposed to be achieved by the current returnToCurrentPage, but that seems broken. And having two buttons gives users more of a choice anyway; some browsers support shortcuts to open things in new tabs, but not all of them do.
This uses the appropriate SVG icon from FontAwesome Free, which seems like a pretty decent option, but filled in metabrainz#666 grey. The 16px CSS setting for imgs inside 'buttons' is 15 years old or so and I'm not sure where it even applies nowadays (I didn't find any cases in a quick search but I might have missed some). 15px does seem to work a lot better with our button style though, as suggested by aerozol. Having the text as aria-label, title *and* alt for the image might be overkill, but there's no kill like overkill.
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. Some editors wanted to keep the current option for some reason so this adds a second button so that the editor has the choice.In both cases, the checkboxes are cleared when submitting the merge, allowing to easily continue using the page to merge further entries if desired.
This was supposed to be achieved by the current
returnToCurrentPage
, but that seems broken. And having two buttons gives users more of a choice anyway; some browsers support shortcuts to open things in new tabs, but not all of them do.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.