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 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

brainzbot commented May 1, 2017

Can one of the admins verify this patch?

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

This comment has been minimized.

Copy link
@d4rkie

d4rkie May 1, 2017

Contributor

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

Sophist-UK commented May 1, 2017

Agreed.

@mwiencek
Copy link
Member

mwiencek commented May 29, 2017

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

Sophist-UK commented May 29, 2017

@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 {

This comment has been minimized.

Copy link
@mwiencek

mwiencek May 30, 2017

Member

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

This comment has been minimized.

Copy link
@Sophist-UK

Sophist-UK May 30, 2017

Author Contributor

Oh - yes - you are right. Apologies.

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

This comment has been minimized.

Copy link
@Freso

Freso May 30, 2017

Member

Random whitespace? :(

This comment has been minimized.

Copy link
@Sophist-UK

Sophist-UK May 30, 2017

Author Contributor

:-(

Copy link
Contributor

yvanzo left a comment

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

Sophist-UK commented Jun 1, 2017

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

@yvanzo
yvanzo approved these changes Jun 1, 2017
@@ -386,6 +386,10 @@ div.warning img.warning {
margin-top: 0;
}

#sidebar ul.external_links {

This comment has been minimized.

Copy link
@mwiencek

mwiencek Jun 3, 2017

Member

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
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants
You can’t perform that action at this time.