EntityLink cleanup #630
EntityLink cleanup #630
Conversation
| hover = entity.sort_name + bracketed(comment); | ||
| } | ||
|
|
||
| if (entityType === 'artist' || entityType === 'instrument') { | ||
| content = content || entity.l_name; | ||
| if (entity.entityType === 'area') { |
There was a problem hiding this comment.
See 75252bf. I only discovered this bug because of the added types (Flow complained about l_name not existing, which caused me to dig deeper and see that the condition was also wrong). So they're already helping. :)
| title?: string, | ||
| +target?: '_blank', | ||
| }; | ||
| /* eslint-enable sort-keys, flowtype/sort-keys */ |
There was a problem hiding this comment.
I wanted the keys that are only supposed to exist in the anchorProps spread to be grouped together, so I disabled those eslint rules for this block.
|
|
||
| if (showDisambiguation === undefined) { | ||
| showDisambiguation = !hasCustomContent; | ||
| } | ||
|
|
||
| if (entityType === 'artist' && !nonEmpty(hover)) { | ||
| if (entity.entityType === 'artist' && !nonEmpty(hover)) { |
There was a problem hiding this comment.
Since CoreEntityT is now a disjoint union, the entityType check allows Flow to determine that the entity must be an ArtistT inside this block, allowing the entity.sort_name access.
But for some reason this feature only works if you access entity.entityType directly in the conditional, rather than caching it in a variable as we were doing before.
There was a problem hiding this comment.
Is that limitation documented or reported?
There was a problem hiding this comment.
I found facebook/flow#3932 and facebook/flow#4828 (though they're related to destructuring the values more specifically, I guess the same logic would apply).
It might also be related to refinement invalidation in that Flow can't know if entity.entityType has changed value since it was stored in the constant...so it may be a way to enforce soundness.
| @@ -35,7 +37,11 @@ const DeletedLink = ({name, allowNew}) => { | |||
| const Comment = ({className, comment}) => ( | |||
| <Frag> | |||
| {' '} | |||
| <span className={className}>({isolateText(comment)})</span> | |||
| <span className={className}> | |||
| {'('} | |||
|
|
||
| const NoInfoURL = ({url, allowNew}) => ( | ||
| <Frag> | ||
| <a href={url}>{url}</a> | ||
| {' '} | ||
| <DeletedLink name={'[' + l('info') + ']'} allowNew={allowNew} /> | ||
| <DeletedLink allowNew={allowNew} name={'[' + l('info') + ']'} /> |
There was a problem hiding this comment.
Maybe we can modify bracketed to do all 3 kinds of brackets depending on flags passed and default to round?
| @@ -209,7 +224,7 @@ const EntityLink = ({ | |||
| } | |||
|
|
|||
| if (infoLink) { | |||
| parts.push(' [', <a href={infoLink} key="info">{l('info')}</a>, ']') | |||
| parts.push(' [', <a href={infoLink} key="info">{l('info')}</a>, ']'); | |||
There was a problem hiding this comment.
this can also be replaced by the refactored bracketed util?
mwiencek
force-pushed
the
entitylink-cleanup
branch
from
c33b18f to
ab7c48a
Compare
|
@sambhav Agreed about using bracketed, though the result isn't 100% pretty: ab7c48a About removing the space from the strings as mentioned in the commit: I'm not sure if it's necessary for some languages to control the presence of the space, and if that's the reason it was included, or just because it was convenient to have because most callers want the space. Should we change the translated string from e.g. |
|
Yes, this space has to be moved out of this function eventually. It happens that bracketed text always comes after another text, thus there always is a space before. IMHO, it is overfactorization. I don’t think it is necessary to any language, at least it is not useful to any of currently supported language, not to mention it does not support RTL languages. |
mwiencek
force-pushed
the
entitylink-cleanup
branch
from
ab7c48a to
bf2518a
Compare
See the comments in metabrainz#630
mwiencek
force-pushed
the
entitylink-cleanup
branch
from
bf2518a to
b5fc65b
Compare
See the comments in metabrainz#630
Fixes a couple issues in the previous code: * It should've been checking for areas, not artists. Artists don't have translated names (not counting aliases), but areas which are countries do. You can see this is the behavior of the original TT macros, too: areas and instruments default to l_name for their content. * We don't serialize any l_name property. We have to get the translated name from the appropriate gettext domain.
This is what this actually intends to test.
See the comments in metabrainz#630
Allow passing a "type" option to bracketed specifying which type of brackets to use. Defaults to "()". Use this in EntityLink.
mwiencek
force-pushed
the
entitylink-cleanup
branch
from
b5fc65b to
230cc07
Compare
to distinguish from JS function `bracketed` for further conversion.
|
Yes, good call. Thanks! |
See the comments in metabrainz#630
The EntityLink component is used by a ton of pages and components, so it'd be very beneficial to have types for it to ensure it's being used correctly (and is implemented correctly).
I added types and performed some misc. fixes/cleanups.