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

Links in the main section do not have a visible outline #756

Open
harneetsahi opened this issue Mar 27, 2025 · 9 comments · May be fixed by #766
Open

Links in the main section do not have a visible outline #756

harneetsahi opened this issue Mar 27, 2025 · 9 comments · May be fixed by #766
Assignees
Labels
Bug Something isn't working

Comments

@harneetsahi
Copy link

Most appropriate sections of the p5.js website?

Home

What is your operating system?

Mac OS

Web browser and version

Firefox 136, Chrome 134.0.6998.89, Safari 18.3.1

Actual Behavior

When using tab to navigate the website, links like 'Reference', 'Examples', 'Community', 'Donate', and 'Download Library' do not have a visible outline. I'm not sure if this is the intended behaviour but it's difficult to visually know if I'm on the correct link without using a voiceover assistant or a mouse.

In Chrome, you can see a minor outline difference if you lock your eyes on the link when tabbing back and forth. The search bar also does not have an outline.

In Firefox and Safari, there is no visible outline at all around the links. But, the search bar outline works correctly.

Image

Expected Behavior

A visible outline should appear around the links so it's easy to spot which link is in the focus state.

Image

Steps to reproduce

  • Use "tab" to navigate the website.

Would you like to work on the issue?

I can work on it if it's approved.

@harneetsahi harneetsahi added the Bug Something isn't working label Mar 27, 2025
@ksen0
Copy link
Member

ksen0 commented Mar 28, 2025

Hi @harneetsahi thanks for spotting this! Please feel free to work on it.

It might be useful to check if there's a style that's being ignored in dark-mode; this was a bug recently.

@ksen0 ksen0 assigned ksen0 and harneetsahi and unassigned ksen0 Mar 28, 2025
@harneetsahi
Copy link
Author

harneetsahi commented Mar 28, 2025

Thank you @ksen0. Last I checked, the outline was specified for these links matching their background color across all themes and overriding the browser default. But I'll take a better look tonight to check if any particular styles are not applying correctly.

@harneetsahi
Copy link
Author

harneetsahi commented Mar 31, 2025

Hi @ksen0
I'm running into some issues while working on it. Some tests keep failing and it happens before I even make any code changes. Every time I run npm run test it deletes contributor-docs (sometimes upto 99 files, sometimes none at all but test is still failing). I'm not sure if I'm doing something wrong.

I'm using node v20.18.1 but I've tried with higher and lower versions, it's the same issue.
I'm using macOS.
Steps I took before it breaks are: fork the repo > clone my fork > npm install.
At this point, npm run lint is throwing warnings but npm run test is what's preventing me from moving forward.

Image
Image

I wonder if any specific node version is required? Is there something else I'm missing? Please let me know if you can take a look. I'm not sure if I should push up changes if tests are not passing.

@ksen0
Copy link
Member

ksen0 commented Mar 31, 2025

Hi @harneetsahi I can't seem to reproduce the problem. I have (on macOS):

node -v
v20.17.0

and I npm install npm run test without problems. Is it just the tests, are you able to npm run dev/build?

If you can't make the local build work, you can also make a PR and when you push code there the pipeline will run the automated tests.

If you do find the local build issue, it would be really great if you shared your finding as I'm sure it would be helpful to include in the docs for future contributors.

@harneetsahi
Copy link
Author

Yes, npm run dev works. I can see the changes I make locally. I tried it again just now.

  • Initially, it shows 9 test files passed, 36 tests passed, no errors.
  • I change 1 line of code (styling of a link from "outline" to "border")
  • Re-ran the tests.
  • 9 test files passed, 36 tests passed, 1 error (all contributor-docs are deleted)
  • I reset everything to the original state
  • Still 9 test files passed, 36 tests passed, 1 error.

It keeps pointing to "test/scripts/build-reference.test.ts" and Missing script: "docs"

@harneetsahi
Copy link
Author

@ksen0 Do you recommend I still make a PR and if the automated tests fail then it can be rejected and maybe someone else can make those changes? It doesn't have to be me. I can push the changes so you can review the code and see if it's good enough before someone implements it

@ksen0
Copy link
Member

ksen0 commented Mar 31, 2025

Hi @harneetsahi it's alright, you can make a PR anyway and I can also take a look (might take a few days but it's not a problem)

But looking at this: "I reset everything to the original state" How do you execute the revert? By reverting the line or eg git stash?

I change 1 line of code (styling of a link from "outline" to "border")

Hmmm, could it maybe be a namespace collision problem with the word "border" by any chance?

@harneetsahi
Copy link
Author

I clicked on "Discard all changes" in Source Control ._. I didn't commit any unwanted file changes.
I'm not sure if it is a namespace collision because I used it within the context of class name, which should be scoped to the selector.

@harneetsahi harneetsahi linked a pull request Mar 31, 2025 that will close this issue
@harneetsahi
Copy link
Author

I may have wasted a lot of your time ._. apologies
I don't know why the tests are throwing errors locally but it seems the automated ones passed.
If any changes are needed, please let me know.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants