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.

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

UI component switch toggle #924

Merged
merged 6 commits into from
Jul 15, 2024
Merged

UI component switch toggle #924

merged 6 commits into from
Jul 15, 2024

Conversation

Angi-Kinas
Copy link
Collaborator

@Angi-Kinas Angi-Kinas commented Jul 2, 2024

Description

This PR introduces a new UI Input component "SwitchToggleComponent", which is able to toggle between options similar to radio buttons.

The component should be accessible with the keyboard. Please verify this in the Storybook.

Screenshots

image

Quality Assurance Checklist

  • Commit history is devoid of any merge commits and readable to facilitate reviews
  • If new logic ⚙️ is introduced: unit tests were added
  • If new user stories 🤏 are introduced: E2E tests were added
  • If new UI components 🕹️ are introduced: corresponding stories in Storybook were created
  • If breaking changes 🪚 are introduced: add the breaking change label
  • If bugs 🐞 are fixed: add the backport <release branch> label
  • The documentation website 📚 has received the love it deserves

Copy link
Contributor

github-actions bot commented Jul 2, 2024

Affected libs: feature-editor, ui-inputs, feature-dataviz, feature-record, feature-router, feature-search, feature-map, ui-elements, feature-notifications, feature-catalog, ui-catalog, ui-search, ui-layout,
Affected apps: metadata-editor, datafeeder, demo, metadata-converter, datahub, webcomponents, search, map-viewer,

  • 🚀 Build and deploy storybook and demo on GitHub Pages
  • 📦 Build and push affected docker images

Copy link
Contributor

github-actions bot commented Jul 2, 2024

📷 Screenshots are here!

@coveralls
Copy link

Coverage Status

coverage: 81.266% (-1.8%) from 83.112%
when pulling 580718f on UI-component-switch-toggle
into fe59f17 on main.

Copy link
Contributor

github-actions bot commented Jul 3, 2024

GitHub Pages links:

  • (Documentation)[https://geonetwork.github.io/geonetwork-ui/UI-component-switch-toggle/docs/]
  • (Demo & web components)[https://geonetwork.github.io/geonetwork-ui/UI-component-switch-toggle/demo/]
  • (UI components storybook)[https://geonetwork.github.io/geonetwork-ui/UI-component-switch-toggle/storybook/demo/]

Copy link
Collaborator

@LHBruneton-C2C LHBruneton-C2C left a comment

Choose a reason for hiding this comment

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

Looking good, working great (tested on the storybook github page).

You just need to change the component to standalone for this PR to be approved.

@Output() selectedValue = new EventEmitter<SwitchToggleOption>()

onChange(selectedOption: SwitchToggleOption) {
this.options.forEach((option) => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Small possible improvement here (nothing critical): as you can't select several options at the same time, you don't need to loop over the whole array, you can stop as soon as you've found the selected element.
To do this, you can use the array.find method.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I tried this and it works! I'm just wondering how it sets the other options.checked to false? Maybe it does not need to because as you already said, it is not possible to select multiple at a time. But how does the component know which to style differently if all options.checked are true?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I didn't read the Angular Material toggle implementation, but most probably they rely on some behavior (native radio button or custom one) to make sure that if one option is checked, the other ones are unchecked. It almost never makes sense to explicitly set an option to unchecked, unless you allow your toggle switch to be in a an "all unchecked" state.

Comment on lines 15 to 20
@Component({
selector: 'gn-ui-switch-toggle',
templateUrl: './switch-toggle.component.html',
styleUrls: ['./switch-toggle.component.css'],
changeDetection: ChangeDetectionStrategy.OnPush,
})
Copy link
Collaborator

Choose a reason for hiding this comment

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

This component should be created as standalone.

@@ -46,6 +48,7 @@ import { ImageInputComponent } from './image-input/image-input.component'
CopyTextButtonComponent,
CheckboxComponent,
SearchInputComponent,
SwitchToggleComponent,
Copy link
Collaborator

Choose a reason for hiding this comment

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

All the changes in this file won't be needed anymore when switching to standalone component.

change logic, remove imports in ui-inputs module
@Angi-Kinas
Copy link
Collaborator Author

Thank you for the review @LHBruneton-C2C :-)
I changed the component to be standalone and simplified the logic a bit as you suggested. Can you have another look?

@coveralls
Copy link

Coverage Status

coverage: 81.314% (-1.8%) from 83.112%
when pulling e50812e on UI-component-switch-toggle
into fe59f17 on main.

Copy link
Collaborator

@LHBruneton-C2C LHBruneton-C2C left a comment

Choose a reason for hiding this comment

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

Looking great, thanks for the fixes!

@Angi-Kinas Angi-Kinas merged commit 58b3c2e into main Jul 15, 2024
9 checks passed
@Angi-Kinas Angi-Kinas deleted the UI-component-switch-toggle branch July 15, 2024 07:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants