Skip to content

Conversation

danji90
Copy link
Contributor

@danji90 danji90 commented Jun 12, 2024

How to

In WKP we need to track certain events for the SBB, including the BaseLayerSwitcher and Zoom buttons.
A prop was added to trigger a callback function on click of the buttons in the components

Others

  • It's not a hack or at least an unauthorized hack :).
  • The images added are optimized.
  • Everything in ticket description has been fixed.
  • The author of the MR has made its own review before assigning the reviewer.
  • The title means something for a human being and follows the conventional commits specification.
  • The title contains [WIP] if it's necessary.
  • Labels applied. if it's a release? a hotfix?
  • Tests added.

@danji90 danji90 added the hotfix label Jun 12, 2024
@danji90 danji90 requested a review from oterral June 12, 2024 16:19
@danji90 danji90 self-assigned this Jun 12, 2024
Copy link

vercel bot commented Jun 12, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
react-spatial ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jun 13, 2024 11:27am

Copy link
Contributor

@oterral oterral left a comment

Choose a reason for hiding this comment

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

I prefer that you add multiple onXXXClick prop instead of one that you called everywhere.

It will avoid to maage the 2nd parameter which likely is only useful in application, totally useless in react-spatial.

onCloseButttonClick
onLayerButtonClick
onSwitcherButtonClick

onZoomInButtonClick
onZoomOutButtonClick

@danji90
Copy link
Contributor Author

danji90 commented Jun 13, 2024

I prefer that you add multiple onXXXClick prop instead of one that you called everywhere.

It will avoid to maage the 2nd parameter which likely is only useful in application, totally useless in react-spatial.

onCloseButttonClick onLayerButtonClick onSwitcherButtonClick

onZoomInButtonClick onZoomOutButtonClick

makes sense. Done

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

Successfully merging this pull request may close these issues.

2 participants