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

Component center img #143

Merged
merged 23 commits into from
Jul 11, 2024
Merged

Component center img #143

merged 23 commits into from
Jul 11, 2024

Conversation

YUUU23
Copy link
Contributor

@YUUU23 YUUU23 commented Jun 21, 2024

  • In docs/assets/center_img.jsx: Create new component <CenterImg /> that follows JSX original issue
<div textAlign="center">
  <image src={src} alt="alt" />
</div>

suggested (passed in as props) with additional prop item imgStyle specifying any style attributes (such as height) the image should take

  • in mdx files in /docs: those that contain image tags (originally <img />) are changed to <CenterImg /> for uniformed centering image component

@YUUU23 YUUU23 self-assigned this Jun 21, 2024
@YUUU23 YUUU23 linked an issue Jun 21, 2024 that may be closed by this pull request
Copy link

github-actions bot commented Jun 27, 2024

Visit the preview URL for this PR (updated for commit 834e37b):

https://ccv-honeycomb-docs--pr143-component-center-img-01ozszps.web.app

(expires Thu, 18 Jul 2024 15:08:18 GMT)

🔥 via Firebase Hosting GitHub Action 🌎

Sign: d403a110fede5b0308678a87537bf61d0597aef6

Copy link
Member

@RobertGemmaJr RobertGemmaJr left a comment

Choose a reason for hiding this comment

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

Can you do the git merge main fix here as well?

Comment on lines 14 to 19
let imgStyleString;
if (props.imgStyle != undefined && Object.keys(props.imgStyle).length === 0) {
imgStyleString = {};
} else {
imgStyleString = props.imgStyle;
}
Copy link
Member

Choose a reason for hiding this comment

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

A great way of doing prop delegation is by using the spread operator

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see! This looks really nice and makes more sense to keep things clean. I tried to modify the current CenteredImage component to use this! Thank you!

* @returns a component for uniformally centering images in the doc
*/

export default function CenterImg(props) {
Copy link
Member

Choose a reason for hiding this comment

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

This is an example of how you could set up the component with prop delegation

Suggested change
export default function CenterImg(props) {
export default function CenterImg({src, alt, ...delegated}) {

* imgStyle: object, any image styles (props can be passed in as: `imgStyle={{ maxHeight: "600px", border: "solid" }}`)
* @returns a component for uniformally centering images in the doc
*/

Copy link
Member

Choose a reason for hiding this comment

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

There's an extra empty line here

Suggested change

Copy link
Member

Choose a reason for hiding this comment

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

A couple semantic things here

  1. I would call this component CenteredImage as component are things ("A centered image") and not an action ("a function that centers an image") - hope that makes sense!
  2. After this could you rename the file itself to be CenteredImage.jsx? Different companies may have different naming conventions but it's generally best to give the file the same exact name as the component

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see! This makes so much sense; thank you so much :) ! Just went in and refactored the semantics.

@YUUU23 YUUU23 requested a review from RobertGemmaJr July 9, 2024 16:33
Comment on lines 23 to 27
return (
<div style={{ textAlign: "center" }}>
<img src={src} alt={alt} style={imgStyleString} />
</div>
);
Copy link
Member

Choose a reason for hiding this comment

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

If you destructure the delegated prop object into the image it will handle all of the style props for you!

Suggested change
return (
<div style={{ textAlign: "center" }}>
<img src={src} alt={alt} style={imgStyleString} />
</div>
);
return (
<div style={{ textAlign: "center" }}>
<img src={src} alt={alt} {...delegated} />
</div>
);

Copy link
Member

Choose a reason for hiding this comment

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

This means you can delete lines 13-21

@YUUU23 YUUU23 requested a review from RobertGemmaJr July 9, 2024 21:09
Copy link
Member

@RobertGemmaJr RobertGemmaJr left a comment

Choose a reason for hiding this comment

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

So sorry but I'm gonna be super annoying 🫣

Can you move the CenteredImage.jsx file from docs/assets/ to /components/? You'll have to create the components folder. From there you can import it as :

import CenteredImage from '@site/components/CenteredImage';

So sorry I didn't catch this sooner!

Copy link
Member

@RobertGemmaJr RobertGemmaJr left a comment

Choose a reason for hiding this comment

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

🚀 Thanks for all the little fixes!

@YUUU23 YUUU23 merged commit abffc88 into main Jul 11, 2024
3 checks passed
@YUUU23 YUUU23 deleted the component-center-img branch July 11, 2024 20:34
@RobertGemmaJr RobertGemmaJr added the 3.4 Versioning: Issue in regards to version 3.4 release label Jul 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3.4 Versioning: Issue in regards to version 3.4 release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Component for a centered image
2 participants