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-9323: Fix long URLs distorting page layout. #505

Merged
merged 7 commits into from Jun 12, 2017

Conversation

Sophist-UK
Copy link
Contributor

@Sophist-UK Sophist-UK commented May 1, 2017

Long URLs do not wrap and cause page distortion e.g. in https://musicbrainz.org/artist/f2b1690b-7b7a-4364-9764-098ecf87f794 a Wikipedia url with a very long #id caused the sidebar to take c. 80% of page width.

This proposed fix is applied to all <a> tags on every page. Further selectors can be added to e.g. restrict this to the sidebar if that is prefered. e.g.

&#sidebar {
    word-break: break-all;
}

Resolves https://tickets.metabrainz.org/browse/MBS-9323

@brainzbot
Copy link
Member

Can one of the admins verify this patch?

@@ -18,6 +18,7 @@ body {
a {
color:@link-default;
text-decoration: none;
word-break: keep-all;
Copy link
Contributor

Choose a reason for hiding this comment

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

word-break: break-all; should be a better choice here, IMO. works in most browsers, including webkit-based ones

@d4rkie
Copy link
Contributor

d4rkie commented May 1, 2017

keep-all will distort the UI in the opposite direction (making it overscroll instead). break-all will keep the characters in and not break the layout

@Sophist-UK
Copy link
Contributor Author

Agreed.

@mwiencek
Copy link
Member

Unfortunately this overrides our other word-break rules and causes every link to break mid-word (which is ugly) where they'd otherwise break between words. I think this rule should be specific to the sidebar links, which either don't have words or are already very short if they do.

@Sophist-UK
Copy link
Contributor Author

@mwiencek I will make it specific - as offered in the original description.

@@ -18,6 +18,10 @@ body {
a {
color:@link-default;
text-decoration: none;

&#sidebar {
Copy link
Member

Choose a reason for hiding this comment

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

That matches a#sidebar, i.e. a link with id="sidebar". :) You need an a rule inside the #sidebar block instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh - yes - you are right. Apologies.

@@ -18,7 +18,7 @@ body {
a {
color:@link-default;
text-decoration: none;

Copy link
Member

Choose a reason for hiding this comment

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

Random whitespace? :(

Copy link
Contributor Author

Choose a reason for hiding this comment

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

:-(

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.

Property word-break: break-all of element a is ignored by the current version of Firefox. It should be applied it to one of its ancestors instead, ul.external_links for example.

@Sophist-UK
Copy link
Contributor Author

@yvanzo: Just tested this with Firefox and you are indeed right.

@@ -386,6 +386,10 @@ div.warning img.warning {
margin-top: 0;
}

#sidebar ul.external_links {
Copy link
Member

Choose a reason for hiding this comment

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

We have a block for #sidebar ul.external_links right below this on line 398, so it'd be good to put this rule inside the existing block.

@mwiencek mwiencek merged commit 6713ed7 into metabrainz:master Jun 12, 2017
mwiencek added a commit that referenced this pull request Jul 13, 2017
* master:
  MBS-9403: Output track position in JSON WS
  MBS-9391: Update import_db.sh to retry downloading dumps.
  Remove old/unused deployment files
  Fix User::Edit tests for MBS-9355
  Update User::Edit tests for MBS-9355
  MBS-9355: Disable bio and website of limited users
  MBS-9353: Requires login for /collection and /user
  MBS-9372: Allow notes from voting-disabled editor
  MBS-9323: Fix long URLs distorting page layout. (#505)
  MBS-9369: Support for importing dumps in the test-database image
  Add workaround for packet #104949
  Fix alias merge logic
  MBS-9365: Create event_meta_fk_id for old standalone DBs
  Remove redundant command
  MBS-9342: Fix imports with DBD::Pg 3.6.0
  Remove pg_enable_utf8
  Clarify comment about pg_server_prepare
mwiencek added a commit that referenced this pull request Jul 17, 2017
* beta:
  Fix instrument table names in DropTriggers.sql
  MBS-9403: Output track position in JSON WS
  MBS-9401: Fix sql query to fetch releases in dump-entities-sql.pl
  MBS-9391: Update import_db.sh to retry downloading dumps.
  Remove old/unused deployment files
  Fix User::Edit tests for MBS-9355
  Update User::Edit tests for MBS-9355
  MBS-9355: Disable bio and website of limited users
  MBS-9353: Requires login for /collection and /user
  MBS-9372: Allow notes from voting-disabled editor
  MBS-9323: Fix long URLs distorting page layout. (#505)
  MBS-9369: Support for importing dumps in the test-database image
  Add workaround for packet #104949
  Fix alias merge logic
  MBS-9365: Create event_meta_fk_id for old standalone DBs
  Remove redundant command
  MBS-9342: Fix imports with DBD::Pg 3.6.0
  Remove pg_enable_utf8
  Clarify comment about pg_server_prepare
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
6 participants