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

chore(web): Add LayerSectionField component #534

Closed
wants to merge 12 commits into from

Conversation

atnagata
Copy link
Contributor

@atnagata atnagata commented Jun 29, 2023

@atnagata atnagata requested a review from KaWaite as a code owner June 29, 2023 06:24
@netlify
Copy link

netlify bot commented Jun 29, 2023

Deploy Preview for reearth-web ready!

Name Link
🔨 Latest commit 895434c
🔍 Latest deploy log https://app.netlify.com/sites/reearth-web/deploys/64a260edc67baa0008f19ff9
😎 Deploy Preview https://deploy-preview-534--reearth-web.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.

@github-actions github-actions bot requested a review from pyshx June 29, 2023 06:25
@codecov
Copy link

codecov bot commented Jun 29, 2023

Codecov Report

Merging #534 (895434c) into main (de536e8) will increase coverage by 0.60%.
The diff coverage is 2.24%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #534      +/-   ##
==========================================
+ Coverage   26.65%   27.26%   +0.60%     
==========================================
  Files        1342     1344       +2     
  Lines      144699   144751      +52     
  Branches     3485     3541      +56     
==========================================
+ Hits        38571    39467     +896     
+ Misses     105005   104161     -844     
  Partials     1123     1123              
Flag Coverage Δ
web 24.77% <2.24%> (+0.77%) ⬆️
web-beta 24.77% <2.24%> (+0.77%) ⬆️
web-classic 24.77% <2.24%> (+0.77%) ⬆️
web-utils 24.77% <2.24%> (+0.77%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...eb/src/beta/components/LayerSectionField/index.tsx 0.00% <0.00%> (ø)
web/src/beta/components/LayerSectionField/item.tsx 0.00% <0.00%> (ø)
web/src/beta/components/Icon/icons.ts 100.00% <100.00%> (ø)

... and 25 files with indirect coverage changes

Copy link
Member

Choose a reason for hiding this comment

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

Remember currentColor

icon="layerAdd"
size={14}
color={theme.general.content.main}
onClick={onClickLayerAdd}
Copy link
Member

Choose a reason for hiding this comment

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

Since the LayerAddFrame has padding, it would probably be nice to be able to click more than just the icon, so can you move the onClick to this component? 🙏🏻
Can you also add a :hover: { cursor: "pointer"; } so we know its clickable

Comment on lines 28 to 30
const handleAction = useCallback(() => {
onAction?.(layer.id);
}, [layer.id, onAction]);
Copy link
Member

Choose a reason for hiding this comment

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

I think this needs stopPropagation(). Right now if I click the action button this AND handleActive are executed.

Comment on lines 25 to 27
const handleVisible = useCallback(() => {
onVisible?.(layer.id);
}, [layer.id, onVisible]);
Copy link
Member

Choose a reason for hiding this comment

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

I think this needs stopPropagation(). Right now if I click the visibility button this AND handleActive are executed.

${props => (props.active ? "background: " + props.theme.general.select + ";" : "")}
${props => (props.active ? "border-radius: 4px;" : "")}
&:hover {${props =>
props.active ? "" : "background: " + props.theme.general.bg.weak + "; border-radius: 0px;"}
Copy link
Contributor

Choose a reason for hiding this comment

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

(optional) same as #533 (comment)

Comment on lines +85 to +90
const ItemVisibilityIcon = styled(Icon)`
padding-left: 30%;
padding-right: 33.44%;
padding-top: 30%;
padding-bottom: 27.34%;
`;
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you use flex instead of this padding?

@KaWaite KaWaite closed this Sep 25, 2023
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.

None yet

4 participants