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

Markup/Styling improvements #2887

Draft
wants to merge 9 commits into
base: beta
Choose a base branch
from
Draft

Markup/Styling improvements #2887

wants to merge 9 commits into from

Conversation

phoneticallySAARTHaK
Copy link

  • links in navigation take full width of the parent which makes it difficult to close the accordion unless clicked on the marker. (DMT has fixed this)
  • Remove the empty anchor tag being used as an anchor target as it interferes with DOM tab order, and AT (Assistive Technologies)
  • Contrast issues:
    • link color in dark theme
    • alert color in light theme (Contrast ratio is 5, which is 0.5 more than the required value, but not good enough for low brightness)
  • Align summary element contents
  • Remove tabindex, aria-expanded, role attributes from Index summary element
  • Add code blocks button and permalink focus styles to make them visible when being focused just like on hover

@phoneticallySAARTHaK
Fix minor search input bugs
d0321bc 
- Select text in input when the modal is opened.

- Up/Down arrow keys work like Home/End, changing the caret position, that shouldn't happen, but holding Shift with these keys selects text so it doesn't make sense to block it.

- The usual action of clicking the field is to toggle the listbox, but since here the listbox is always shown, so simply remove the selection/visual focus.
@phoneticallySAARTHaK
Remove animation name dependency from setUpModal
8eef980 
Now it works without having to pass the animation name as a parameter, allowing to change closing animation with just css

- Add more comments clarifying the usage.
- Remove incorrect comment on Modal type about Popover, as popovers are non-modal dialogs.
@phoneticallySAARTHaK
Add focus styling to permalink and code blocks button
dfa01fd
@phoneticallySAARTHaK
Add aria-current attribute to current link
0516233 
Follows [ARIA Breadcrumb Example](https://www.w3.org/WAI/ARIA/apg/patterns/breadcrumb/examples/breadcrumb/)
@phoneticallySAARTHaK
Remove anchorLink and rename anchorLinkIfPresent
959ce25 
- Replace `anchorLink` call with id attribute on its container.
- Replace `anchorLinkIfPresent` with `anchorTargetIfPresent` clarifying its usage. It returns the target/fragment id.

- Style changes: vertically align anchor icon, enable smooth scrolling, add scroll-margin to target.
@phoneticallySAARTHaK
Copy link
Author

phoneticallySAARTHaK commented Mar 3, 2025 •
edited
Loading

Is this a test case failing? I don't get any error on the test package script, not even test:full

@Gerrit0
Copy link
Collaborator

Gerrit0 commented Mar 3, 2025

Yes, maybe try merging in beta? I added some tests for the default theme rendering last night.

@phoneticallySAARTHaK
Copy link
Author

Oh, of course. PR changes theme output.

@Gerrit0
Copy link
Collaborator

Gerrit0 commented Mar 3, 2025

Changes so far LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
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
@phoneticallySAARTHaK @Gerrit0