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

OCLOMRS-1054:Avoid implying SYNONYM is a name type #749

Merged
merged 2 commits into from
Oct 3, 2021

Conversation

suruchee
Copy link
Contributor

JIRA TICKET NAME:

Avoid implying SYNONYM is a name type

Summary:

Name type of Synonym is displayed at the top in grey italic text with a bracket

Comment on lines 151 to 157
{nameType.label.includes("Synonym") ? (
<div className={classes.menuItemSynonym}>
({nameType.label})
</div>
) : (
<div>{nameType.label}</div>
)}
Copy link
Member

Choose a reason for hiding this comment

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

Two simplifications:

Suggested change
{nameType.label.includes("Synonym") ? (
<div className={classes.menuItemSynonym}>
({nameType.label})
</div>
) : (
<div>{nameType.label}</div>
)}
<div className={nameType.label === "Synonym" && classes.menuItemSynonym}>
({nameType.label})
</div>
  1. We only want this to apply to "Synonym" not "Custom Synonym" (since presumbably that needs some kind of value).
  2. We can do this with a single div

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @ibacher, I tried this with a single div but ended up with an error that says: Type 'string | false' is not assignable to type 'string | undefined'.   Type 'false' is not assignable to type 'string | undefined'.

Copy link
Member

Choose a reason for hiding this comment

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

Try this expression instead:

nameType.label === "Synonym" ? classes.menuItemSynonym : undefined

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ibacher , it's updated please have a look.

Comment on lines 271 to 272
{ value: "FULLY_SPECIFIED", label: "Fully Specified" },
{ value: "null", label: "Synonym" }, // value here should be null but inputs html can't handle null as a value
{ value: "FULLY_SPECIFIED", label: "Fully Specified" },
Copy link
Member

Choose a reason for hiding this comment

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

I'm less sure about this... "Fully Specified" should definitely be the first option we display when creating a new name, so maybe we just don't change this for now?

Copy link
Member

Choose a reason for hiding this comment

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

Incidentally, I think this is why there's a test failure. The test assumes that the first name entered is always a FSN and for any concept in any language we always require at least one FSN

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, for catching this test failure, I will revert it.

Comment on lines 50 to 51
color: "#A9A9A9",
fontStyle: "italic"
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure about the font style. Also, since we use the Material Design System for this app, we should consult their guidelines about color. In particular, for disabled text, instead of adjusting the color directly, the Material guidelines suggest using 38% percent opacity.

That means this should probably be:

Suggested change
color: "#A9A9A9",
fontStyle: "italic"
opacity: 0.38

@suruchee
Copy link
Contributor Author

@ibacher, I have updated with your suggestions please have a look.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.02%) to 46.634% when pulling 9a9a1cf on suruchee:OCLOMRS-1054 into da18b22 on openmrs:master.

Copy link
Member

@ibacher ibacher left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks @suruchee!

@ibacher ibacher merged commit 87fae50 into openmrs:master Oct 3, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants