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

fix: #438 External URL icon has strange underline #439

Merged
merged 13 commits into from
Dec 17, 2024
Merged

Conversation

Tishasoumya-02
Copy link
Contributor

image

Copy link
Collaborator

@stevepiercy stevepiercy left a comment

Choose a reason for hiding this comment

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

Looks much better. Thank you!

@stevepiercy
Copy link
Collaborator

@Tishasoumya-02 would you please add a news item? Thank you!

packages/volto-light-theme/news/439.bugfix Outdated Show resolved Hide resolved
packages/volto-light-theme/news/439.bugfix Outdated Show resolved Hide resolved
@stevepiercy
Copy link
Collaborator

Although unrelated to this PR, there is a failing build now when it was not failing before at https://github.com/kitconcept/volto-light-theme/actions/runs/12153651828/job/33901673414#step:8:324. I have no idea how to fix that. I compared a successful build to a failed one, and see that failed builds mention this:

> @plone/[email protected] build /home/runner/work/volto-light-theme/volto-light-theme/core/packages/components
> tsup && pnpm build:css

CLI Building entry: src/index.ts
CLI Using tsconfig: tsconfig.json
CLI tsup v8.3.5
CLI Using tsup config: /home/runner/work/volto-light-theme/volto-light-theme/core/packages/components/tsup.config.ts
CLI Target: es2022
CLI Cleaning output folder
CJS Build start
ESM Build start
CJS dist/index.cjs 162.26 KB
CJS ⚡️ Build success in 149ms
ESM dist/index.js 133.94 KB
ESM ⚡️ Build success in 156ms
DTS Build start
Error: src/components/Checkbox/Checkbox.tsx(15,9): error TS2322: Type '{ children: (string | number | boolean | Element | ReactElement<any, string | JSXElementConstructor<any>> | Iterable<ReactNode> | ((values: CheckboxRenderProps & { ...; }) => ReactNode) | null | undefined)[]; }' is not assignable to type '{ children?: ReactNode; }'.

My guess is that it is related to this change in Volto from last month, as the timing of the failure and change are pretty close. @sneridagh can you enlighten us?

@danalvrz
Copy link
Contributor

danalvrz commented Dec 5, 2024

@Tishasoumya-02 thanks for looking into this! It looks so much better already. Since you are working on it, could it be possible to implement the icon in a way that it can also be customized using the --link-foreground-color prop? Right now, if I want the links to behotpink the icon remains blue as specified in the data URL:

Screenshot 2024-12-05 at 11 44 37 a m

/cc @iRohitSingh @sneridagh

@Tishasoumya-02
Copy link
Contributor Author

@Tishasoumya-02 thanks for looking into this! It looks so much better already. Since you are working on it, could it be possible to implement the icon in a way that it can also be customized using the --link-foreground-color prop? Right now, if I want the links to behotpink the icon remains blue as specified in the data URL:

Screenshot 2024-12-05 at 11 44 37 a m /cc @iRohitSingh @sneridagh

Okay will look into it.

@Tishasoumya-02
Copy link
Contributor Author

Tishasoumya-02 commented Dec 10, 2024

@danalvrz , I made the changes, but I was not able to use the --link-foreground-color prop as that's a dynamic var and was not able to interpolate with the url , instead I used a static var $icon: (some color) ,and inserted in the url. Will this be okay? If you any suggestions, please let me know.

@stevepiercy
Copy link
Collaborator

Aside from how to set the color, please consider accessibility with whatever colors you choose for links. Contrast is extremely important. Here's a decent guide for link color accessibility https://webaim.org/blog/wcag-2-0-and-link-colors/.

hotpink #ff69b4 on white #ffffff is too low contrast.

https://webaim.org/resources/contrastchecker/

I also think there is some guidance about having links with different colors, except of course their active, focus, hover, visited states.

I think that having two different colors for internal and external links is unusual and may cause some accessibility issues. Unless WCAG says it is definitely OK, I would avoid it.

@danalvrz
Copy link
Contributor

Aside from how to set the color, please consider accessibility with whatever colors you choose for links. Contrast is extremely important. Here's a decent guide for link color accessibility https://webaim.org/blog/wcag-2-0-and-link-colors/.

hotpink #ff69b4 on white #ffffff is too low contrast.

https://webaim.org/resources/contrastchecker/

I also think there is some guidance about having links with different colors, except of course their active, focus, hover, visited states.

I think that having two different colors for internal and external links is unusual and may cause some accessibility issues. Unless WCAG says it is definitely OK, I would avoid it.

@stevepiercy yes, color contrast is very important and having a user-friendly contrast checking feature is on the list of things to work on next. Specially with the new color system. The idea is not be to have different colors for external vs internal links, but to be able to change all links to a specific color using the prop --link-color. It is particularly useful if, for example, you are building a site with a dark theme and need a more contrasting color for your links than the default blue.

Copy link
Contributor

@danalvrz danalvrz left a comment

Choose a reason for hiding this comment

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

@Tishasoumya-02 we should maybe update the news entry to mention the current implementation for the icon. Thanks for looking into it!

@danalvrz danalvrz requested a review from stevepiercy December 16, 2024 15:14
@danalvrz danalvrz merged commit a2f87d4 into main Dec 17, 2024
25 checks passed
@danalvrz danalvrz deleted the externallink branch December 17, 2024 15:55
@danalvrz danalvrz restored the externallink branch December 17, 2024 15:55
@danalvrz danalvrz deleted the externallink branch December 17, 2024 15:55
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.

4 participants