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 ReferenceLine/ReferenceArea to use a store, prop parity with ReferencePoint #2184

Merged
merged 40 commits into from
Jul 17, 2024

Conversation

zachstence
Copy link
Member

@zachstence zachstence commented Jul 3, 2024

Description

Checklist

  • For UI or styling changes, I have added a screenshot or gif showing before & after
  • I have added a changeset
  • I have added to the docs where applicable
  • I have added to the VS Code extension where applicable

@zachstence zachstence self-assigned this Jul 3, 2024
Copy link

changeset-bot bot commented Jul 3, 2024

🦋 Changeset detected

Latest commit: f94e6d4

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 5 packages
Name Type
@evidence-dev/core-components Minor
@evidence-dev/evidence Major
my-evidence-project Patch
@evidence-dev/components Patch
evidence-test-environment Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

Copy link

netlify bot commented Jul 3, 2024

Deploy Preview for next-docs-evidence ready!

Name Link
🔨 Latest commit f94e6d4
🔍 Latest deploy log https://app.netlify.com/sites/next-docs-evidence/deploys/6696c22847548e00082e8932
😎 Deploy Preview https://deploy-preview-2184--next-docs-evidence.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link

netlify bot commented Jul 3, 2024

Deploy Preview for evidence-development-workspace ready!

Name Link
🔨 Latest commit f94e6d4
🔍 Latest deploy log https://app.netlify.com/sites/evidence-development-workspace/deploys/6696c228eb02f30008dd1eea
😎 Deploy Preview https://deploy-preview-2184--evidence-development-workspace.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link

netlify bot commented Jul 3, 2024

Deploy Preview for evidence-test-env ready!

Name Link
🔨 Latest commit f94e6d4
🔍 Latest deploy log https://app.netlify.com/sites/evidence-test-env/deploys/6696c228282c510008823d3c
😎 Deploy Preview https://deploy-preview-2184--evidence-test-env.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@zachstence zachstence force-pushed the references-refactor-and-prop-parity branch from c9c20af to 467258a Compare July 5, 2024 19:03
@zachstence zachstence marked this pull request as ready for review July 10, 2024 19:25
@zachstence
Copy link
Member Author

@ItsMeBrianD Let's have a conversation about what we want to do with the stores. I'm not sure that converting them to classes (or 1 class) like EvidenceMap.js is the way to go:

  • There is no parent reference component that manages children (like Map and Point, MapArea, etc). This isn't a technical limitation, but moreso a pattern I don't think fits here
  • The map markers aren't reactive, and the references are. We need to update their position/styling when the incoming props change. This would require some additional thought of how to handle that with a class/classes.
  • Seems unnecessarily complex, all we're really doing is converting from our props to ECharts for the references, whereas map does a lot more

Copy link
Member

@archiewood archiewood left a comment

Choose a reason for hiding this comment

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

Overall works well aside from one unintentional breaking change

We will also need to update the docs reference for RefLine/Area where additional props are available

@zachstence
Copy link
Member Author

zachstence commented Jul 15, 2024

We will also need to update the docs reference for RefLine/Area where additional props are available

@archiewood This is done, see the diff for annotations.md

@zachstence zachstence force-pushed the references-refactor-and-prop-parity branch from 3f2ad54 to a891660 Compare July 15, 2024 20:28
@zachstence zachstence force-pushed the references-refactor-and-prop-parity branch from a891660 to 8f5c57c Compare July 16, 2024 16:12
@zachstence zachstence force-pushed the references-refactor-and-prop-parity branch from 8f5c57c to a9207a6 Compare July 16, 2024 16:17
@zachstence zachstence requested a review from archiewood July 16, 2024 18:03
Copy link
Member

@archiewood archiewood left a comment

Choose a reason for hiding this comment

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

Looks great. Easy review with storybook

@zachstence zachstence merged commit fb92c78 into next Jul 17, 2024
12 checks passed
@zachstence zachstence deleted the references-refactor-and-prop-parity branch July 17, 2024 16:53
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