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.

Sign up for GitHub

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

Jump to bottom

Remove focus-visible polyfill #38323

Merged
merged 1 commit into from
May 17, 2023
Merged

Remove focus-visible polyfill #38323

merged 1 commit into from
May 17, 2023

Conversation

pulsejet
Copy link
Member

Supercedes and closes #35384
(that PR is already approved but in a different fork so can't rebase easily)

@pulsejet pulsejet requested review from CarlSchwan and Pytal May 16, 2023 18:52
@pulsejet pulsejet enabled auto-merge May 16, 2023 18:53
@pulsejet pulsejet added this to the Nextcloud 28 milestone May 16, 2023
@pulsejet pulsejet added 4. to release Ready to be released and/or waiting for tests to finish javascript labels May 16, 2023
@Pytal Pytal requested review from a team, susnux and Fenn-CS and removed request for a team May 16, 2023 18:59
@susnux
Copy link
Contributor

susnux commented May 16, 2023 •
edited
Loading

Unsure about this, from @skjnldsv question whether we still need this:
https://caniuse.com/mdn-css_selectors_focus-visible

Most Browsers are fine, but only supported on Safari 15+.
At least our browserslist still lists Safari 14 as supported.

So I think we have to wait for the NC27 branch off and then merge this for NC28.

@pulsejet
Copy link
Member Author

Most Browsers are fine, but only supported on Safari 15+. At least our browserslist still lists Safari 14 as supported.

So I think we have to wait for the NC27 branch off and then merge this for NC28.

Sure, I did add it to the NC28 milestone, not 27.
This does have some impact on performance though, and Safari <15 is at 0.95% browser only (also this is just a minor visual regression).

@pulsejet pulsejet disabled auto-merge May 17, 2023 00:14
@skjnldsv
Copy link
Member

28 it is yep

@pulsejet @susnux
Remove focus-visible polyfill
db8d0b5 
Supercedes and closes #35384

Signed-off-by: Varun Patil <varunpatil@ucla.edu>
@pulsejet
Copy link
Member Author

master is 28 now

@susnux
Copy link
Contributor

susnux commented May 17, 2023

Yes, just waiting for CI to complete :)

@pulsejet pulsejet merged commit 545b3b3 into master May 17, 2023
37 checks passed
@pulsejet pulsejet deleted the pulsejet/focus-visible branch May 17, 2023 23:42
@ArtificialOwl ArtificialOwl mentioned this pull request Jul 13, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@susnux susnux susnux approved these changes

@Fenn-CS Fenn-CS Fenn-CS approved these changes

@skjnldsv skjnldsv skjnldsv approved these changes

@Pytal Pytal Pytal approved these changes

@CarlSchwan CarlSchwan Awaiting requested review from CarlSchwan

Assignees
No one assigned
Labels
4. to release Ready to be released and/or waiting for tests to finish javascript
Projects
None yet
Milestone
Nextcloud 28
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@pulsejet @susnux @skjnldsv @Fenn-CS @Pytal