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

Refactor ToggleButton and ToggleGroup #3509

Closed
wants to merge 50 commits into from
Closed

Conversation

dnlkoch
Copy link
Member

@dnlkoch dnlkoch commented Sep 6, 2023

Description

This simplifies the internal handling of the ToggleButton and ToggleGroup components.

TODOs:

  • migration guide; TLDR:
    • it's required to control the pressed state in the parent component
    • the name prop has been renamed to value
    • onToggle has been removed in favor of onChange
  • GeolocationButton and MeasureButton aren't implementing the needed control of the pressed state, this will be done after they have been transformed to functional components (in a follow up PR)
    • fixed for the GeolocationButton ✔️
    • fixed for the MeasureButton ✔️
  • the tests are still failing and need to be adjusted (in this PR) ✔️
  • there are some linting errors left (in this PR) ✔️

Please review @terrestris/devs.

Related issues or pull requests

--

Pull request type

  • Bugfix
  • Feature
  • Dependency updates
  • Code style update (formatting, renaming)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • Documentation content changes
  • Other (please describe)

Do you introduce a breaking change?

  • Yes
  • No

Checklist

  • I understand and agree that the changes in this PR will be licensed under the
    BSD-2-Clause.
  • I have followed the guidelines for contributing.
  • The proposed change fits to the content of the Code of Conduct.
  • I have added or updated tests and documentation, and the test suite passes (run npm test locally).
  • I have added a screenshot/screencast to illustrate the visual output of my update.

dnlkoch and others added 30 commits September 4, 2023 11:24
BREAKING CHANGE: Removes the deprecated DigitizeButton in favour of the DrawButton
Remove the deprecated `DigitizeButton` in favour of the `DrawButton`
BREAKING CHANGE: mappify, loadify, isVisibleComponent HOCs as well as MapProvider class were removed
BREAKING CHANGE: Panel, Window and Titlebar components are not existing anymore
Remove Window, Panel and Titlebar component
refactor: remove toolbar component
refactor: remove userchip component
Eslint: homogenize / sort imports using simple-sort-plugin
src/Button/GeoLocationButton/GeoLocationButton.tsx Outdated Show resolved Hide resolved
src/Button/MeasureButton/MeasureButton.tsx Outdated Show resolved Hide resolved
if (onClick) {
onClick(evt);

if (evt.defaultPrevented) {
Copy link
Member

Choose a reason for hiding this comment

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

Why is this preventing onChange but not onClick?

Copy link
Member

Choose a reason for hiding this comment

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

And why is it preventing onChange only if onClick is set?

Copy link
Member Author

Choose a reason for hiding this comment

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

This might be a refactor-artifact. Will double check, but I assume it can be removed.

@dnlkoch dnlkoch marked this pull request as draft September 19, 2023 07:52
@dnlkoch dnlkoch force-pushed the refactor-toggle-button branch 2 times, most recently from 728ec8f to 7ddba57 Compare September 26, 2023 11:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants