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 ImageBlock #511

Closed
wants to merge 10 commits into from

Conversation

atnagata
Copy link
Contributor

@atnagata atnagata commented Jun 20, 2023

@atnagata atnagata requested a review from KaWaite as a code owner June 20, 2023 02:12
@netlify
Copy link

netlify bot commented Jun 20, 2023

Deploy Preview for reearth-web ready!

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

@codecov
Copy link

codecov bot commented Jun 20, 2023

Codecov Report

Merging #511 (e914ffe) into main (de536e8) will increase coverage by 0.44%.
The diff coverage is 9.30%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #511      +/-   ##
==========================================
+ Coverage   26.65%   27.10%   +0.44%     
==========================================
  Files        1342     1321      -21     
  Lines      144699   142442    -2257     
  Branches     3485     3542      +57     
==========================================
+ Hits        38571    38602      +31     
+ Misses     105005   102791    -2214     
+ Partials     1123     1049      -74     
Flag Coverage Δ
web 24.78% <9.30%> (+0.79%) ⬆️
web-beta 24.78% <9.30%> (+0.79%) ⬆️
web-classic 24.78% <9.30%> (+0.79%) ⬆️
web-utils 24.78% <9.30%> (+0.79%) ⬆️

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

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

... and 125 files with indirect coverage changes


type Props = {
src: NonNullable<ImgHTMLAttributes<HTMLImageElement>["src"]>;
align?: "left" | "center" | "right"; // default center
Copy link
Member

Choose a reason for hiding this comment

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

You shouldn't need the comments here in the props.
Can you

  • remove the comments
  • Inside the props destructuring(line 15) assign defaults (ie. ({src, align="center", fit="cover", .. .. . .. })
  • Lastly update the styled component's props to expect a value for fit and align always and remove the fallbacks

Copy link
Contributor

Choose a reason for hiding this comment

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

The code has changed.
�Above code no longer exists.

Comment on lines 10 to 11
if (src) {
return (
Copy link
Member

Choose a reason for hiding this comment

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

Like the Video block , please use a ternary operator with one return 🙇🏻

}
return (
<BlankImageBox>
<Icon icon={"image"} color={"#2e2e2e"} size={32} />
Copy link
Member

@KaWaite KaWaite Jun 29, 2023

Choose a reason for hiding this comment

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

Don't need { } for strings
so icon="image" color="#2e2e2e"

};

const Wrapper = styled.div`
position: absolute;
Copy link
Member

Choose a reason for hiding this comment

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

Don't think you need this

Comment on lines 5 to 6
src: NonNullable<React.ImgHTMLAttributes<HTMLImageElement>["src"]>;
alt?: React.ImgHTMLAttributes<HTMLImageElement>["alt"];
Copy link
Member

Choose a reason for hiding this comment

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

You don't need to type so specifically. For some elements, where there are specific values needed, this is good.
But for something like src and alt, any string is okay, so you can just type these as string

@KaWaite KaWaite closed this Aug 23, 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.

4 participants