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

MBS-12437 / MBS-12438: Limit overflow-wrap even more #2561

Merged
merged 1 commit into from Jun 7, 2022

Conversation

reosarevok
Copy link
Member

@reosarevok reosarevok commented Jun 7, 2022

Fix MBS-12437 / MBS-12438

Still a bunch of things are wrapping that probably should not, so let's do this the other way around and start from the minimum
amount of wrapped things. This wraps only entity headers (where anything too long will break the interface for no good use), and URL entity links, which can get really long, both on EntityLink and in the Edit URL edit display.
Edit note texts and tags on tag lists already wrap in what seems to be a useful way without complaints, so leaving those in too.

Examples that should still wrap:

Examples that no longer wrap, but do in beta:

@reosarevok reosarevok added the Regression/Beta Bugs that are either on beta or new regressions and should be checked ASAP label Jun 7, 2022
Copy link
Contributor

@yvanzo yvanzo left a comment

Choose a reason for hiding this comment

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

I could test the added examples locally through adaptive view.

The two examples that no longer wrap are not regressions compared to production at least.

It isn’t clear what is reverted and what is new. Splitting into two separate commits can help with.
It isn’t obvious either which previous commit(s) or pull request(s) are referred to.

LGTM otherwise.

@reosarevok
Copy link
Member Author

Originally, #2516 made it so the whole site is allowed to wrap, but that was too messy. Then (the version currently in beta, #2557), every <a> was allowed to wrap. Now, only the specific cases I wrote down on the commit message are allowed to wrap, with everything else behaving like it used to. It's not ideal, since it will still cause issues with super long words in places like tables, but it's the best I could come up with without actively checking for long words on render and adding a class to cause wrapping only if they are present (which seems very inefficient).

@yvanzo
Copy link
Contributor

yvanzo commented Jun 7, 2022

Thanks for the references and details but it is not for me, please add these to the commit message and 🚢 it!

Even after commit 4f47d6a limited
overflow-wrap to just <a> elements and edit notes, there are
still a bunch of things wrapping that probably should not,
and a lot of it seems to happen even when there's nothing that is
really long enough to require wrapping in the first place,
especially in tables with lots of columns.
As such, this commit reverts the generic overflow-wrap for <a>
and takes a bit-by-bit approach starting from the minimum
amount of wrapped things: entity headers (where anything too long
will break the interface for no good use), and URL entity links
(which can get really long), both on EntityLink and in the
Edit URL edit display.
The added wrapping for edit note texts hasn't caused complaints
so far, and tags on tag lists already had overflow-wrap for a long
time now without complaints, so leaving those in too.
Copy link
Contributor

@yvanzo yvanzo left a comment

Choose a reason for hiding this comment

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

Just double-checked that the force-push did not alter the content of the patch from the above.

Copy link
Member

@mwiencek mwiencek left a comment

Choose a reason for hiding this comment

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

Fixes the issues in the tickets for me and I like the new approach.

@@ -35,7 +35,7 @@ const EntityHeader = ({
subHeading,
}: Props): React.Element<typeof React.Fragment> => (
<>
<div className={headerClass}>
<div className={'entityheader ' + headerClass}>
Copy link
Member

Choose a reason for hiding this comment

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

any reason url-entity-link is hyphenated but entityheader isn't?

Copy link
Member Author

Choose a reason for hiding this comment

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

Just because we already had non-hyphenated headerClasses here, so it seemed consistent to keep this like that too. But maybe we should hyphenate all...

Copy link
Member

Choose a reason for hiding this comment

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

OK, just wondering. :) No need to fix them here.

@reosarevok reosarevok merged commit d011540 into metabrainz:beta Jun 7, 2022
@reosarevok reosarevok deleted the MBS-12332-fix-2 branch June 7, 2022 16:07
reosarevok added a commit that referenced this pull request Jun 7, 2022
* beta:
  Update POT files using the production database
  Update translations from Transifex
  MBS-12437: Limit overflow-wrap even more (#2561)
  Use flushSync to avoid render lag (fixes MBS-12424) (#2559)
  Update translations from Transifex
  MBS-12420: Support comma-separated IDs for edit search types again (#2556)
  MBS-12416: Avoid wrapping in menu links
  MBS-12415: Stop most interface wrapping
  Update POT files using the production database
  Update translations from Transifex
  MBS-12328: Add #content to entity/Edits for artwork display (#2502)
  MBS-12336: Validate that Predicate::Set is being passed integers (#2509)
  MBS-12333: Block smart links: share.amuse.io (#2501)
  Run all sql before enabling replication triggers (#2539)
  MBS-12412: Fail gracefully if requesting an invalid ID in edit-cover-art (#2547)
  MBS-12387: Add titles to URL editor edit buttons (#2550)
  Bump react-table to 7.8.0 (#2551)
  MBS-12407: Add tobarandualchais.co.uk to the other dbs whitelist (#2545)
  MBS-12280 (II): Add target _blank to more edit form links (#2546)
  MBS-12413: Use uri_escape_utf8 on Data::Wikidoc (#2548)
  ExportAllTables.t: remove --nodbmirror2 flags (#2538)
  MBS-12398: Update release_meta.amazon_asin (#2543)
  Bump Flow to 0.179.0
  JSON dumps: ignore more useless primary/foreign keys (#2536)
  MBS-12404: Replace Text::Unaccent (#2542)
  MBS-12353: Actually check proposed ratings are allowed (#2511)
  MBS-8193 / MBS-12332: Wrap absurdly long lines (#2516)
  MBS-11694: Cleanup /intent/user Twitter URLs (#2519)
  MBS-8875: Improve CatNoLooksLikeASIN regexp (#2520)
  MBS-12347: Also show RG artist in autocomplete if type is null (#2517)
  MBS-12385: Add report for digital releases with mail order rels (#2535)
  MBS-12212: Add tests for event filtering
  MBS-12212: Allow filtering event list by setlist contents
  MBS-12212: Add filtering to artists' events tab
  Update react, react-dom to v18 (#2544)
  Bump Flow to 0.178.1
  Upgrade Flow to 0.178.0
  Fix spammy PG warnings in ProcessReplicationChanges
  Remove BundleReplicationPackets
  MBS-4960: Give useful error when checking donation status fails
  MBS-12361: Allow navigation from donation check page
reosarevok added a commit that referenced this pull request Jun 7, 2022
* production:
  Update POT files using the production database
  Update translations from Transifex
  MBS-12437: Limit overflow-wrap even more (#2561)
  MBS-12400: fix non-musicbrainz-schema dumps (#2541)
  Sync incremental JSON dumps to trille
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Regression/Beta Bugs that are either on beta or new regressions and should be checked ASAP
Projects
None yet
3 participants