Skip to content

Discussion: TextLink component #2947

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

Open
4 tasks
lindapaiste opened this issue Jan 23, 2024 · 8 comments
Open
4 tasks

Discussion: TextLink component #2947

lindapaiste opened this issue Jan 23, 2024 · 8 comments
Labels
Area: Code Quality For refactoring, cleanup, or improvements to maintainability Area: Design For UI/UX design updates, proposals, or feedback Needs Discussion Requires further conversation or consensus

Comments

@lindapaiste
Copy link
Collaborator

lindapaiste commented Jan 23, 2024

Tasks

  • Decide what our basic link style should look like - particularly the color (see comment re: contrast)
  • Determine the props API for the TextLink component based on how we want to customize the style for specific use-cases
  • Write the TextLink component
  • Rewrite existing links in the app to use TextLink

Increasing Access

We want to keep the codebase clean and keep the design consistent.

Feature enhancement details

We have had many issue recently relating to the visibility of text links throughout the app. I would like to centralize the discussion here.

The recurring problem is that it's not clear to the user that a certain text is a clickable link. We can improve this by changing the default style of the link as well as adding hover styles to make it look more interactive.

I don't want us to add lots of specific CSS to specific component because this is difficult to maintain. I think this is a good opportunity for us to expand our library of reusable core UI components with a TextLink. We might not want to have the exact same styles everywhere, so we'll want to have some props which can control styles.

The goal of this issue is to discuss our API for the TextLink component and finalize what props we want.

Depending on the API that we choose, the code might look like any of these:

<TextLink href={url} underline="hover" color="always">anchor</TextLink>
<TextLink href={url} underline="hover" color="primary">anchor</TextLink>
<TextLink href={url} primary bold>anchor</TextLink>

^ This last one uses boolean props and is concise but probably more confusing.

Ideas for props:

  • underline with values "always", "hover", "none" (what MUI uses)
  • color with values "always", "hover", "none" to apply the theme primary accent color (pink or yellow)
  • color with values "primary", "inherit", etc. to choose from multiple theme colors (what MUI uses)
  • primary as a boolean flag to make it pink (this wouldn't allow for pink-on-hover behavior)
  • bold with values "always", "hover", "none" ("hover"` shouldn't be used here, but keeps it consistent)
  • bold as a boolean flag
  • fontWeight with values "bold" or "inherit"
  • display with values "inline" (default) and "block"

Specific related issues:

References / how others handle this:

@lindapaiste lindapaiste added Area: Design For UI/UX design updates, proposals, or feedback Needs Discussion Requires further conversation or consensus Area: Code Quality For refactoring, cleanup, or improvements to maintainability labels Jan 23, 2024
@PiyushThapaa
Copy link
Contributor

There can be multiple combinations of this possible but,
I think what's more appealing is that making all the links bold or simply an underline and adding theme color on hover.

So the code will look like either :
<TextLink href={url} underline="always" color="hover">anchor</TextLink>
OR
<TextLink href={url} fontWeight="bold" color="hover">anchor</TextLink>
(Correct me if I am wrong)

But if Community don't want this idea...

I think the code should be :
<TextLink href={url} underline="hover" color="primary">anchor</TextLink>

What do you think? @lindapaiste @raclim and others

@Mubashirshariq
Copy link
Contributor

Mubashirshariq commented Jan 24, 2024

<TextLink href={url} underline="hover" color="always">anchor</TextLink>
@lindapaiste @raclim
i think the above one prioritizes user accessibility and visibility. The dynamic underline on hover enhances link recognition, while persistent coloring ensures consistent visibility, contributing to an intuitive and user-friendly design.

@Keshav-0907
Copy link
Contributor

Keshav-0907 commented Jan 24, 2024

Yes indeed, this is a great idea! Introducing a centralized TextLink component will not only enhance the visibility of clickable links but also contribute to a cleaner and more maintainable codebase.

I'm want to actively contribute on implementing this TextLink component. If you have any further suggestions or considerations, feel free to share them!

@rahulrana701
Copy link
Contributor

Hi @lindapaiste, I believe the best approach for this is to utilize the Recoil library. With this library, we can define all the props in a separate file along with the functionalities they will perform. Afterwards, we can simply use the functions provided by this library and utilize whichever prop we need throughout the entire application.

The main advantage of this library is that it will save us from prop drilling and the re-renders that often occur. It achieves this by allowing any component to subscribe only to the specific props they actually need without worrying about parent and child components , and without necessitating the passing of props from parent to child.

This not only reduces unnecessary re-renders but also makes our code much cleaner and easier to manage
If you're interested in pursuing this solution,

I'd be happy to work on this issue. You could assign it to me, and I can get started.

@lindapaiste
Copy link
Collaborator Author

Hi @lindapaiste, I believe the best approach for this is to utilize the Recoil library. With this library, we can define all the props in a separate file along with the functionalities they will perform. Afterwards, we can simply use the functions provided by this library and utilize whichever prop we need throughout the entire application.

The main advantage of this library is that it will save us from prop drilling and the re-renders that often occur. It achieves this by allowing any component to subscribe only to the specific props they actually need without worrying about parent and child components , and without necessitating the passing of props from parent to child.

This not only reduces unnecessary re-renders but also makes our code much cleaner and easier to manage If you're interested in pursuing this solution,

I'd be happy to work on this issue. You could assign it to me, and I can get started.

I’m familiar with Recoil and I think it’s pretty cool. We’re deeply entwined with using Redux as our state management system so I don’t have any plans to change that.

Also I don’t see how Recoil relates to this issue at all. I think you might be misunderstanding the discussion.

The idea is that there are certain props of a TextLink which might be different in various places. For example we might want underlines for links on the Sign In page, which appear in the middle of a sentence and need more distinction, but not want underlines on the lists of links in the About popup. So we will create a TextLink component that has a prop which tells us whether or not to show an underline on that particular link.

@rahulrana701
Copy link
Contributor

rahulrana701 commented Jan 25, 2024

Hi @lindapaiste, I believe the best approach for this is to utilize the Recoil library. With this library, we can define all the props in a separate file along with the functionalities they will perform. Afterwards, we can simply use the functions provided by this library and utilize whichever prop we need throughout the entire application.
The main advantage of this library is that it will save us from prop drilling and the re-renders that often occur. It achieves this by allowing any component to subscribe only to the specific props they actually need without worrying about parent and child components , and without necessitating the passing of props from parent to child.
This not only reduces unnecessary re-renders but also makes our code much cleaner and easier to manage If you're interested in pursuing this solution,
I'd be happy to work on this issue. You could assign it to me, and I can get started.

I’m familiar with Recoil and I think it’s pretty cool. We’re deeply entwined with using Redux as our state management system so I don’t have any plans to change that.

Also I don’t see how Recoil relates to this issue at all. I think you might be misunderstanding the discussion.

The idea is that there are certain props of a TextLink which might be different in various places. For example we might want underlines for links on the Sign In page, which appear in the middle of a sentence and need more distinction, but not want underlines on the lists of links in the About popup. So we will create a TextLink component that has a prop which tells us whether or not to show an underline on that particular link.

Hi @lindapaiste my plan was not to change the state management library I understood the issue very well , my goal was to have recoil library only to control the css of these links and use them wherever we want instead of creating a component for these , but if you want a component for them I am up for that as well.

@lindapaiste
Copy link
Collaborator Author

lindapaiste commented Jan 26, 2024

Responding to #2940 (comment) - I want to centralize the discussion here.

@lindapaiste Hmm, I think the primary accent color might not work because it doesn't pass the color contrast criteria for accessibility.
Screenshot 2024-01-25 at 3 42 00 PM

I think we actually have a lot of elements within the site that definitely don't pass this 😅 particularly, the dark grey text on light grey backgrounds. I've been wondering in terms of appearance, if it might be better to reach out to a designer who can give a clear direction on how all these elements should look that also aligns with these standards?

I think it's important that we follow these best-practices. Honestly I love rules because it removes a lot of subjectivity and helps me make decisions. So I've been reading a bit.

The standard is that when a link is presented inline with other text it should have an underline, regardless of color. That will make it so that links are obvious and fix most of the current issues. The underline is not necessary when we have an entire element that's a link. It's also acceptable to use bold instead of underline but IMO underline is more easily understood as a link.

W3 says:

While some links may be visually evident from page design and context, such as navigational links, links within text are often visually understood only from their own display attributes. Removing the underline and leaving only the color difference for such links would be a failure because there would be no other visual indication (besides color) that it is a link.

The contrast ratio is close enough to passable that maybe we just need a slightly darker variation of the pink to use as a text link color for light mode? At least, it's very close to meeting the AA-level guideline. We have a ratio of 4.2 with #ED225D. It only needs to be 4.5 to pass AA but 7.0 to pass AAA. So AAA would require going with a significantly darker pink (around #AF0E3C) or introducing a secondary accent color. A slight tweak like #E7134F gets us up to 4.5. In dark mode we're at 5.0 so that's passable already.

@raclim
Copy link
Collaborator

raclim commented Jan 30, 2024

@lindapaiste Thanks for looking into this!

I'm down with using the underline as well over bold. I think the editor has a lot more standalone links (like in the About Modal) than inline ones. Maybe for those types of links the underline could show up on hover?

I guess I was feeling a little hesitant about implementing color changes that might stray from p5.js' identity, but the 4.5 #E7134F tweak doesn't feel too different!

As an added note, I think looking at school/university websites would also be great for more references/examples because I think they've been required to meet these standards and are probably already implementing a lot of these (for example, NYU had an initiative to be AA compliant back in 2017).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: Code Quality For refactoring, cleanup, or improvements to maintainability Area: Design For UI/UX design updates, proposals, or feedback Needs Discussion Requires further conversation or consensus
Projects
None yet
Development

No branches or pull requests

6 participants