Skip to content

Conversation

sandbergja
Copy link
Contributor

https://issues.apache.org/jira/browse/SOLR-17936

Description

In the new ui, add a focus indicator around the sidebar entry that has focus.

Here is a demonstration of me using Tab and Shift-Tab to navigate between the various items, and Enter to select the item I want:

navigating between the various side nav options.  selected items have an orange background, the currently focused item has a gray background, and unselected unfocused items have a white background

Solution

Rather than using Tab, which does not have colors for our various states, we use Button, which has all colors except for the Selected state. Material 3's new "Expressive" version also provides an experiment ToggleButton component, but our version of the Material 3 dependency is not new enough to be able to use it (and if I understand correctly, we can't use the very newest Material 3 release, since Compose Multiplatform does not yet support it).

Many thanks to @malliaridis for the guidance getting started, and for suggesting that we try a component other than Tab.

Tests

I tested this manually in both light mode and dark mode, on both desktop and web versions of the new admin UI. I used Tab and Shift+Tab to navigate between the items in the side navigation and confirmed that the item I navigated to was always clearly marked.

Checklist

Please review the following and check all that apply:

  • I have reviewed the guidelines for How to Contribute and my code conforms to the standards described there to the best of my ability.
  • I have created a Jira issue and added the issue ID to my pull request title.
  • I have given Solr maintainers access to contribute to my PR branch. (optional but recommended, not available for branches on forks living under an organisation)
  • I have developed this patch against the main branch.
  • I have run ./gradlew check.
  • I have added tests for my changes.
  • I have added documentation for the Reference Guide

Rather than using Tab, which does not have colors for our various states,
we use Button, which has all colors except for the Selected state.
Material 3's new "Expressive" version also provides an experiment ToggleButton
component, but our version of the Material 3 dependency is not new enough to
be able to use it (and if I understand correctly, we can't use the very newest
material 3 release, since Compose Multiplatform does not yet support it).

With thanks to @malliaridis for the guidance getting started, and for
suggesting that we try a component other than Tab.
Copy link
Contributor

@malliaridis malliaridis left a comment

Choose a reason for hiding this comment

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

Changes look great and work perfectly on both light and dark theme. I tested them on windows and mac, and run the browser version too.

The only thing I would probably look into at some point in the future is to extract the implementation to a reusable Composable (like a SolrToggleButton) once we need a similar "toggle" effect in other places. Maybe then we can also look into additional semantics for screen readers for the selected state. But these are topics that are out of scope in my opinion and would explicit testing.

About testing this, I looked into retrieving the background in a test, but this seems quite difficult without rewriting the library's composables right now (so I think). Only the selected state could be applied via the semantics and retrieved in a test if we do something like modifier = modifier.semantics { this.selected = selected }.

So it is fine for now not to have an explicit test for the background change when focused.

@sandbergja if you give me the final go I will merge this change for you. :)

@sandbergja
Copy link
Contributor Author

Thanks for the review and guidance, @malliaridis ! I think this is ready from my perspective.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants