Copilot commented on code in PR #2923:
URL: https://github.com/apache/sedona/pull/2923#discussion_r3206462978
##########
docs-overrides/partials/alternate.html:
##########
@@ -1,15 +1,23 @@
{% if config.extra.alternate and config.extra.alternate | length > 1 %}
-<nav class="sd-lang-switch" aria-label="{{ lang.t('select.language') }}">
- {% for alt in config.extra.alternate %}
- {%- set is_active = (alt.lang == config.theme.language) -%}
- <a
- href="{{ alt.link | url }}"
- hreflang="{{ alt.lang }}"
- lang="{{ alt.lang }}"
- target="_self"
- class="sd-lang-switch__item{% if is_active %}
sd-lang-switch__item--active{% endif %}"
- {% if is_active %}aria-current="true"{% endif %}
- >{{ alt.name }}</a>
- {% endfor %}
-</nav>
+<details class="sd-lang-switch">
+ <summary class="sd-lang-switch__trigger" aria-label="{{
lang.t('select.language') }}" title="{{ lang.t('select.language') }}">
+ <span class="sd-lang-switch__glyph" aria-hidden="true"><span
class="sd-lang-switch__glyph-cn">文</span><span
class="sd-lang-switch__glyph-en">A</span></span>
+ </summary>
+ <ul class="sd-lang-switch__menu" role="menu">
+ {% for alt in config.extra.alternate %}
+ {%- set is_active = (alt.lang == config.theme.language) -%}
+ <li role="none">
+ <a
+ href="{{ alt.link | url }}"
+ hreflang="{{ alt.lang }}"
+ lang="{{ alt.lang }}"
+ target="_self"
+ role="menuitem"
+ class="sd-lang-switch__link{% if is_active %}
sd-lang-switch__link--active{% endif %}"
+ {% if is_active %}aria-current="true"{% endif %}
+ >{{ alt.name }}</a>
+ </li>
Review Comment:
The ARIA `role="menu"` / `role="menuitem"` pattern isn’t appropriate for a
simple list of navigation links and can reduce accessibility (e.g., screen
readers may stop announcing these as links and users may expect arrow-key menu
behavior that isn’t implemented). Consider removing the `role` attributes and
letting this remain a normal list of links (optionally keep an enclosing `<nav
aria-label=...>`), or implement a fully compliant ARIA menu interaction model
if you want to keep menu roles.
##########
docs-overrides/assets/stylesheets/components/_lang-switch.scss:
##########
@@ -1,50 +1,98 @@
.sd-lang-switch {
+ position: relative;
display: inline-flex;
- align-items: stretch;
- margin: 0 0.4rem;
- padding: 2px;
- border: 1px solid rgba(255, 255, 255, 0.45);
- border-radius: 999px;
- background: rgba(255, 255, 255, 0.08);
- font-size: 0.72rem;
- line-height: 1;
-
- &__item {
+ align-items: center;
+ margin: 0 0.2rem;
+
+ // Hide the default disclosure marker on summary
+ &__trigger::-webkit-details-marker { display: none; }
+ &__trigger { list-style: none; }
+
+ &__trigger {
display: inline-flex;
align-items: center;
- padding: 0.28rem 0.7rem;
- color: rgba(255, 255, 255, 0.85);
- text-decoration: none;
+ justify-content: center;
+ min-width: 1.7rem;
+ height: 1.7rem;
+ padding: 0 0.32rem;
border-radius: 999px;
- font-weight: 600;
- letter-spacing: 0.02em;
+ color: rgba(255, 255, 255, 0.85);
+ cursor: pointer;
transition: background-color 120ms ease, color 120ms ease;
&:hover,
- &:focus {
+ &:focus-visible {
color: #ffffff;
background-color: rgba(255, 255, 255, 0.18);
+ outline: none;
}
+ }
- &--active {
- color: #ca463a;
- background-color: #ffffff;
+ &__glyph {
+ display: inline-flex;
+ align-items: baseline;
+ font-family: -apple-system, "Helvetica Neue", "PingFang SC", "Hiragino
Sans GB", "Microsoft YaHei", sans-serif;
+ font-weight: 600;
+ line-height: 1;
+ letter-spacing: -0.02em;
+ }
- &:hover,
- &:focus {
- color: #ca463a;
- background-color: #ffffff;
- }
- }
+ &__glyph-cn {
+ font-size: 0.95rem;
+ margin-right: 0.05rem;
}
-}
-@media (max-width: 600px) {
- .sd-lang-switch {
- font-size: 0.68rem;
+ &__glyph-en {
+ font-size: 0.7rem;
+ transform: translateY(-0.18rem);
+ font-weight: 700;
+ }
- &__item {
- padding: 0.22rem 0.55rem;
+ &[open] &__trigger {
+ color: #ffffff;
+ background-color: rgba(255, 255, 255, 0.22);
+ }
+
+ &__menu {
+ position: absolute;
+ top: calc(100% + 0.35rem);
+ right: 0;
+ min-width: 8rem;
+ margin: 0;
+ padding: 0.3rem 0;
+ list-style: none;
+ background: #ffffff;
+ border-radius: 4px;
+ box-shadow: 0 4px 16px rgba(0, 0, 0, 0.18);
+ z-index: 100;
+
+ li { margin: 0; padding: 0; }
+ }
+
+ &__link {
+ display: block;
+ padding: 0.4rem 0.9rem;
+ color: rgba(0, 0, 0, 0.78);
+ text-decoration: none;
+ font-size: 0.72rem;
+ font-weight: 500;
+ line-height: 1.3;
+ white-space: nowrap;
+
+ &:hover,
+ &:focus {
+ color: #ca463a;
+ background-color: rgba(202, 70, 58, 0.08);
+ outline: none;
+ }
+
+ &--active {
+ color: #ca463a;
+ font-weight: 700;
}
}
}
+
+// Close the dropdown when clicking outside — relies on the click-away
+// hint via `tabindex` on the body and `:focus-within`. Native <details>
+// stays open until summary is clicked again, which is acceptable.
Review Comment:
The comment about closing the dropdown on click-away via `tabindex` on
`<body>` and `:focus-within` is misleading: there’s no corresponding markup/CSS
implementing that behavior, and native `<details>` generally stays open until
the summary is toggled again. Please either remove/adjust this comment to
reflect actual behavior, or add the missing implementation if click-away close
is intended.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
