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

Ensure visibility of focus-ring in Firefox #827

Conversation

RONAK-AI647
Copy link
Contributor

@RONAK-AI647 RONAK-AI647 commented Nov 14, 2024

Description

Making sure the documentation focus-ring is visible in Firefox(earlier it was only visible in chrome).

Issue addressed #206

Addresses #PR# HERE#827

Before/after screenshots

Screen.Recording.2024-11-14.225136.-.Copy.mp4

Changelog

Steps to test

  1. Step 1
  2. Step 2
  3. ...

(optional) Implementation notes

At a high level, how did you implement this?

Does this introduce any tech-debt items?

Testing checklist

  • Contributor has fully tested the PR manually
  • If there are any front-end changes, before/after screenshots are included
  • Critical and brittle code paths are covered by unit tests
  • The change is described in the changelog section above

Reviewer guidance

  • Is the code clean and well-commented?
  • Are there tests for this change?
  • Are all UI components LTR and RTL compliant (if applicable)?
  • Add other things to check for here

Comments

I think now its visible in both browsers clearly,

@RONAK-AI647
Copy link
Contributor Author

@MisRob ,please help me here, I don't know whats wrong with my linter, it always fails here , in my local server it shows "done in secs."

@MisRob MisRob self-requested a review November 15, 2024 09:21
@MisRob MisRob self-assigned this Nov 15, 2024
Copy link
Member

@MisRob MisRob left a comment

Choose a reason for hiding this comment

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

Thanks @RONAK-AI647! I confirm this works well in both Firefox and Chrome, and the change from focus to focus-visible have further benefits. Just one cleanup note.

Regarding the linting trouble, I am fine to lint and push before we merge, however, would you mind doing a recording of what you are experiencing so I can understand better? Perhaps we need to fix something.

On my end, it seems to work, this is what I see:

Screenshot from 2024-11-15 20-08-17

docs/assets/main.scss Outdated Show resolved Hide resolved
@RONAK-AI647
Copy link
Contributor Author

When i run the yarn lint-fix command in my terminal ,this is the output

Screen.Recording.2024-11-16.073500.mp4

Here I thought I might have the browser update issue , but that warning shows in your terminal as well @MisRob .
The linter test yet does not pass here. what should i do?

@MisRob
Copy link
Member

MisRob commented Nov 18, 2024

When i run the yarn lint-fix command in my terminal ,this is the output

That's interesting, @RONAK-AI647. I don't know what's the cause. Would you like to open an issue so we can start tracking it? I noticed some other contributors facing similar troubles. I'd be grateful if you could include recording or description of the following

(1) Do some change that a linter should fix (e.g. remove ;)
(2) Then run yarn lint-fix
(3) Then show git status

It'd be great if you could be as specific as possible and include what change should be fixed and wasn't. Also information about your OS and yarn version.

@MisRob
Copy link
Member

MisRob commented Nov 18, 2024

Regarding this PR, I will do the fix and then merge. Thanks a lot :)!

@MisRob MisRob merged commit 3654a0b into learningequality:develop Nov 18, 2024
8 checks passed
learning-equality-bot bot pushed a commit that referenced this pull request Nov 18, 2024
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.

2 participants