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

feat(css): style outline #26

Merged
merged 1 commit into from
Aug 12, 2023
Merged

feat(css): style outline #26

merged 1 commit into from
Aug 12, 2023

Conversation

mrtnvgr
Copy link
Contributor

@mrtnvgr mrtnvgr commented Aug 12, 2023 •
edited
Loading

Before

After

isunjn
isunjn requested changes Aug 12, 2023
View reviewed changes
Copy link
Owner

@isunjn isunjn left a comment •
edited
Loading

Choose a reason for hiding this comment

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

This will cause accessibility problems.
The default outline style doesn't "look good" but is essential for web accessibility (and lighthouse scores).
Prefer change the outline styles to make it looks good rather than remove it.

@mrtnvgr mrtnvgr changed the title fix(css): disable outline on buttons fix(css): set --primary-color as outline color for buttons Aug 12, 2023
@mrtnvgr mrtnvgr changed the title fix(css): set --primary-color as outline color for buttons feat(css): style outline for buttons Aug 12, 2023
isunjn
isunjn requested changes Aug 12, 2023
View reviewed changes
Copy link
Owner

@isunjn isunjn left a comment

Choose a reason for hiding this comment

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

You may want change <a> outline style too

@mrtnvgr mrtnvgr force-pushed the disable-button-outline branch 2 times, most recently from 9ac5454 to e6aee52 Compare August 12, 2023 14:49
@mrtnvgr mrtnvgr changed the title feat(css): style outline for buttons feat(css): style outline Aug 12, 2023
isunjn
isunjn requested changes Aug 12, 2023
View reviewed changes
Copy link
Owner

@isunjn isunjn left a comment

Choose a reason for hiding this comment

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

  • should place these in 'base' section, not homepage
  • use :fouces-visible instead, see :focus vs :focus-visible
  • change width to 1.5px as the theme use 1.5px for header bottom line and link bottom line

isunjn
isunjn reviewed Aug 12, 2023
View reviewed changes
sass/main.scss Outdated Show resolved Hide resolved
@isunjn isunjn merged commit 584bff0 into isunjn:main Aug 12, 2023
@mrtnvgr mrtnvgr deleted the disable-button-outline branch August 12, 2023 15:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@isunjn isunjn isunjn approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@mrtnvgr @isunjn