-
Notifications
You must be signed in to change notification settings - Fork 20
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
fix: layer button component bugs #3540
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
react-components/src/components/RevealToolbar/LayersContainer/CadModelLayersContainer.tsx
Outdated
Show resolved
Hide resolved
react-components/src/components/RevealToolbar/LayersContainer/Image360LayersContainer.tsx
Outdated
Show resolved
Hide resolved
react-components/src/components/RevealToolbar/LayersContainer/PointCloudLayersContainer.tsx
Outdated
Show resolved
Hide resolved
react-components/src/components/RevealToolbar/LayersContainer/PointCloudLayersContainer.tsx
Outdated
Show resolved
Hide resolved
react-components/src/components/RevealToolbar/LayersContainer/elements.ts
Show resolved
Hide resolved
react-components/src/components/RevealToolbar/LayersContainer/types.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like Christopher already gave you a big list of things to do 😅 I'll just add some nitpick:
- When inside a 360 image and toggling only that 360 set, the viewer does not exit that image
- Still feel like it should be possible to deduplicate the code across layer buttons 🤔 Something along the lines of wrapping every model in types implementing a common interface to get/set toggle etc. But not necessary for this PR
react-components/src/components/RevealToolbar/LayersContainer/CadModelLayersContainer.tsx
Outdated
Show resolved
Hide resolved
….com/cognitedata/reveal into pramodcog/fix-layer-button-component
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me! Apart from the cogs.js-issues, at least :P
react-components/src/components/RevealToolbar/LayersContainer/Image360LayersContainer.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some more minor comments
react-components/src/components/RevealToolbar/LayersContainer/types.ts
Outdated
Show resolved
Hide resolved
react-components/src/components/RevealToolbar/LayersContainer/Image360LayersContainer.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still have the "needing to double click" first time issue. But I'll assume you will follow this up. Other than that, it looks good.
It is bug in cogs.js, the CDS team have acknowledge the bug, will follow up on it |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍
Type of change
Jira ticket 📘
https://cognitedata.atlassian.net/browse/UX-1126
Description 📝
Bug fixes listed in UX-1126 and some more edge cases handled
Example._.Toolbar._.LayersContainer.-.Docs.Storybook.-.Google.Chrome.2023-08-01.16-31-20.mp4
How has this been tested? 🔍
In Storybook
Test instructions ℹ️
cd react-components && yarn && yarn storybook
Select
LayersContainer
exampleChecklist ☑️