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

🏳️ Remove Flag from Undefined Words #315

Open
wants to merge 2 commits into
base: prod
Choose a base branch
from

Conversation

ovlb
Copy link
Collaborator

@ovlb ovlb commented Sep 1, 2020

As a step towards #312, this PR removes flags from all undefined words in the table of content.

This PR also adds tests to the ToC flag logic, to better document exceptions and desired outcomes, and remove fragility from the template files.

@ovlb ovlb added Type · Frontend Frontend work Status · Needs Review Needs editing/translation/code review labels Sep 1, 2020
@ovlb ovlb requested a review from tatianamac September 1, 2020 19:32
@tatianamac
Copy link
Collaborator

tatianamac commented Sep 1, 2020

Thank you, @ovlb ! Very helpful. As part of this PR, I think it makes sense to:

  1. Remove all flags from the table of content level, and move the flag warnings down into part of the definition where it applies. That way, terms like OCD (which stemmed the curiosity around the flags), won't be flagged at the top tier.

Separate but related edits, which I can open as a separate PR:

  1. Add flags at the lower, relevant levels within their definition. For example, add the medically appropriative aspect of a term in its own flagged section. Terms will then get their flags, but with added context, which I think is a win-win. I'm happy to provide more context and/or open another PR around adding this as it's a non-trivial lift.
  2. Turn hover underline to white or black in the content warning box, as it currently fails contrast as red.

Content warning box with hover activated; background is mustard, underline is red

@tatianamac tatianamac added Status · In Progress and removed Status · Needs Review Needs editing/translation/code review labels Sep 1, 2020
@tatianamac tatianamac linked an issue Sep 1, 2020 that may be closed by this pull request
@ovlb
Copy link
Collaborator Author

ovlb commented Sep 1, 2020

Remove all flags from the table of content level, and move the flag warnings down into part of the definition where it applies. That way, terms like OCD (which stemmed the curiosity around the flags), won't be flagged at the top tier.

If I read this correctly, the ToC should show no flags at all thereafter?

Add flags at the lower, relevant levels within their definition. For example, add the medically appropriative aspect of a term in its own flagged section. Terms will then get their flags, but with added context, which I think is a win-win. I'm happy to provide more context and/or open another PR around adding this as it's a non-trivial lift.

Are you thinking about expanding the flag front-matter object with this needed context or integrating the flag via placeholders or sth similar to the definition content?

We can use this PR for this fix, too. Otherwise we’d need to remove the linked issue, I guess, as the PR fixes only a part of the needed steps.

Turn hover underline to white or black in the content warning box, as it currently fails contrast as red.

Good catch. I’ll update this.

@tatianamac
Copy link
Collaborator

If I read this correctly, the ToC should show no flags at all thereafter?

Correct. I think that context-less, it causes more issues than it prevents. It also solves the issue of some terms having multiple flags. The visual indicator it causes can be overalarmist.

Are you thinking about expanding the flag front-matter object with this needed context or integrating the flag via placeholders or sth similar to the definition content?

It's a bit difficult to provide over text, but essentially, "redesigning" each of the sections to accommodate the flags. For example, "medical appropriation" would come with the general warning, and any additional insights. Think of it as a "If medical appropriation exists, include this module."

The content warning flag is the closest thing we have to this right now.

We can use this PR for this fix, too. Otherwise we’d need to remove the linked issue, I guess, as the PR fixes only a part of the needed steps.
I think that we can link multiple issues and check off individual tasks manually, no? In general I'm more of a fan of smaller, more concise PRs. We can update the description of the original issue in order to accommodate?

@tatianamac tatianamac mentioned this pull request Sep 2, 2020
@ovlb
Copy link
Collaborator Author

ovlb commented May 26, 2022

@tatianamac Just found this age-old branch. I guess this is something we still want to do? I finally have some time again to allocate to Self-Defined and can finish this off.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

🐛 Define and reassess flags
2 participants