MBS-9248: Don't show artist name in hover if same as sort name #458

reosarevok · 2017-02-11T09:45:56Z

Should solve MBS-9248 by checking if the sortname is the same as the name and not printing the name if so.

brainzbot · 2017-02-11T09:45:57Z

Can one of the admins verify this patch?

reosarevok · 2017-02-11T09:46:44Z

root/components/common-macros.tt

@@ -350,8 +350,12 @@ END -%]
    IF namevar AND action == 'show' AND content != ''
            AND (noescape ? html_unescape(content.remove('</?span\b[^>]*>')) : content) != entity.name;
        options.unshift('name_variation');
-        hover = hover != '' ? l('{name} – {additional_info}', { name => entity.name, additional_info => hover })
-                            : entity.name;
+        IF (entity.sort_name && entity.sort_name == entity.name);


Not 100% sure if entity.sort_name is needed, let me know if I should just get rid?

Depends on what you want. As it is, it checks whether the sort name is "0" or something else.

Meaning if entity.sort_name is literally the string "0", it'll be "false" because perl. I think the intent here is to guard against entities that don't have a sort_name field at all, so it should be no longer needed if the check is moved as suggested below.

gentlecat · 2017-02-11T11:24:23Z

@brainzbot, add to whitelist.

chirlu · 2017-02-11T14:20:37Z

I’m against this “improvement”; see the ticket for rationale.

Furthermore, how it is implemented in this PR is broken in several ways, but I would prefer not to waste time on writing review comments unless I actually have to.

Freso · 2017-02-12T11:15:50Z

If you mention it, you have to. Even if the PR is dropped, @reosarevok (or other people watching along) cannot learn and improve from the mistakes they're not told about. So either don't mention that it is flawed, or back it up by arguing why.

chirlu

In addition to the specific issues mentioned, changes to the corresponding JS functions are missing completely.

chirlu · 2017-02-12T19:02:29Z

root/components/common-macros.tt

@@ -350,8 +350,12 @@ END -%]
    IF namevar AND action == 'show' AND content != ''
            AND (noescape ? html_unescape(content.remove('</?span\b[^>]*>')) : content) != entity.name;
        options.unshift('name_variation');
-        hover = hover != '' ? l('{name} – {additional_info}', { name => entity.name, additional_info => hover })
-                            : entity.name;
+        IF (entity.sort_name && entity.sort_name == entity.name);


This is the wrong place to do changes specific for artists, as this is a generic macro that is used for all main entities.

I'm not sure I follow the reasoning there. The main part of this is already in the generic macro, despite only applying to entities with name variations (so, right now, artists, unless I'm forgetting some). How is it any different to also put here the code that applies to entities with name variations which happen to have a sort name equal to their name?

We also use namevar when recordings are linked to using a track name. So I think it's correct to do the change in link_artist instead, because artists are the only entities with sort names (well, non-alias ones).

chirlu · 2017-02-12T19:03:35Z

root/components/common-macros.tt

@@ -350,8 +350,12 @@ END -%]
    IF namevar AND action == 'show' AND content != ''
            AND (noescape ? html_unescape(content.remove('</?span\b[^>]*>')) : content) != entity.name;
        options.unshift('name_variation');
-        hover = hover != '' ? l('{name} – {additional_info}', { name => entity.name, additional_info => hover })
-                            : entity.name;
+        IF (entity.sort_name && entity.sort_name == entity.name);


Depends on what you want. As it is, it checks whether the sort name is "0" or something else.

chirlu · 2017-02-12T19:10:34Z

root/components/common-macros.tt

+        ELSE;
+            hoverprint = l('{name} – {additional_info}', { name => entity.name, additional_info => hover });
+        END;
+        hover = hover != '' ? hoverprint : entity.name;


I refrain from commenting on this for the moment since it will have to look different when moved to its proper place.

mwiencek · 2017-02-14T15:48:59Z

root/components/common-macros.tt

@@ -350,8 +350,12 @@ END -%]
    IF namevar AND action == 'show' AND content != ''
            AND (noescape ? html_unescape(content.remove('</?span\b[^>]*>')) : content) != entity.name;
        options.unshift('name_variation');
-        hover = hover != '' ? l('{name} – {additional_info}', { name => entity.name, additional_info => hover })
-                            : entity.name;
+        IF (entity.sort_name && entity.sort_name == entity.name);


Meaning if entity.sort_name is literally the string "0", it'll be "false" because perl. I think the intent here is to guard against entities that don't have a sort_name field at all, so it should be no longer needed if the check is moved as suggested below.

mwiencek · 2017-02-14T16:04:35Z

root/components/common-macros.tt

@@ -350,8 +350,12 @@ END -%]
    IF namevar AND action == 'show' AND content != ''
            AND (noescape ? html_unescape(content.remove('</?span\b[^>]*>')) : content) != entity.name;
        options.unshift('name_variation');
-        hover = hover != '' ? l('{name} – {additional_info}', { name => entity.name, additional_info => hover })
-                            : entity.name;
+        IF (entity.sort_name && entity.sort_name == entity.name);


We also use namevar when recordings are linked to using a track name. So I think it's correct to do the change in link_artist instead, because artists are the only entities with sort names (well, non-alias ones).

mwiencek · 2017-02-14T16:08:28Z

root/static/scripts/common/components/EntityLink.js

@@ -145,7 +145,9 @@ const EntityLink = (props = {}) => {

    if (nameVariation) {
      if (hover) {
-        hover = l('{name} – {additional_info}', {name: entity.name, additional_info: hover});
+        if !(entity.name == entity.sortname) {


A ! outside the parens is not valid JavaScript. :P

Try (entity.name !== entity.sortname) instead.

Also, sortname should be sortName.

reosarevok · 2017-04-10T20:03:04Z

We agreed during the meeting to close this and talk about the issue in Jira further.

reosarevok commented Feb 11, 2017

View reviewed changes

reosarevok force-pushed the mbs-9248 branch from 84e46e9 to d243e38 Compare February 12, 2017 11:25

chirlu suggested changes Feb 12, 2017

View reviewed changes

reosarevok force-pushed the mbs-9248 branch from d243e38 to e08fec5 Compare February 13, 2017 20:30

mwiencek requested changes Feb 14, 2017

View reviewed changes

mwiencek reviewed Feb 14, 2017

View reviewed changes

MBS-9248: Don't show artist name in hover if same as sort name

551d0cb

reosarevok force-pushed the mbs-9248 branch from e08fec5 to 551d0cb Compare February 16, 2017 10:02

reosarevok closed this Apr 10, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MBS-9248: Don't show artist name in hover if same as sort name #458

MBS-9248: Don't show artist name in hover if same as sort name #458

reosarevok commented Feb 11, 2017 •

edited

brainzbot commented Feb 11, 2017

reosarevok Feb 11, 2017

chirlu Feb 12, 2017

mwiencek Feb 14, 2017

gentlecat commented Feb 11, 2017

chirlu commented Feb 11, 2017

Freso commented Feb 12, 2017

chirlu left a comment

chirlu Feb 12, 2017

reosarevok Feb 12, 2017

mwiencek Feb 14, 2017

chirlu Feb 12, 2017

chirlu Feb 12, 2017

mwiencek Feb 14, 2017

mwiencek Feb 14, 2017

mwiencek Feb 14, 2017

mwiencek Feb 14, 2017

reosarevok commented Apr 10, 2017

MBS-9248: Don't show artist name in hover if same as sort name #458

MBS-9248: Don't show artist name in hover if same as sort name #458

Conversation

reosarevok commented Feb 11, 2017 • edited

brainzbot commented Feb 11, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gentlecat commented Feb 11, 2017

chirlu commented Feb 11, 2017

Freso commented Feb 12, 2017

chirlu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

reosarevok commented Apr 10, 2017

reosarevok commented Feb 11, 2017 •

edited