-
Notifications
You must be signed in to change notification settings - Fork 40
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
Add inline media controls for the Image component #269
Add inline media controls for the Image component #269
Conversation
|
||
return ( | ||
<StyledComponentContext cacheKey="tenup-component-image"> | ||
<InlineControlsStyleWrapper {...style} {...rest}> |
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.
Would this also work as a singling if the image tag?
I'd love to not add an additional div around the image :)
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.
@fabiankaegy Think this is where I got confused and am not sure I'm following 😆. Apologies! So, the Figure
component already had the figure
wrapper before the change:
const Figure = (props) => {
const { children, style, ...rest } = props;
return (
<figure style={{ position: 'relative', ...style }} {...rest}>
{children}
</figure>
);
};
I basically just swapped it for a styled component that also renders a figure.
Should there not be other conditional log in the main component? Or am I missing something completely.
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.
@ncoetzer Okay I may have misunderstood the code then.
What I was trying to get at is that the end markup should look like this:
<figure>
<img />
<div>
<!-- Insert anything else here -->
</div>
</figure>
and not:
<figure>
<div>
<img />
<!-- Insert anything else here -->
</div>
</figure>
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.
@fabiankaegy Aah okay cool. In that case, yes, that is how it currently works and outputs the code.
However, for any existing instances of this component, it would also wrap their <img>
tags inside a <figure>
, which is a breaking change if I understood things correctly. So, I'm thinking of changing the logic in the main component to conditionally check if inline controls are needed, and if so, wrap the <img>
in a <figure>
. Otherwise, it will just output the <img>
directly as it currently does. We could do this with any other feature placed behind a flag as well.
Of course, this new API also accommodates creating custom children
, so the user would be able to override anything we have with anything they need.
Thoughts?
🎉 A new testing version of this package has been published to NPM. You can install it with |
Size Change: +3.01 kB (+5%) 🔍 Total Size: 69.1 kB
|
Passing run #673 ↗︎
Details:
Review all test suite changes for PR #269 ↗︎ |
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.
This is such a nice improvement!
e845203
into
feature/include-figure-in-image-component
Description of the Change
This pull request adds all the functionality from #172 into #222
Closes #172
How to test the Change
Please refer to #172
Changelog Entry
Credits
Props @fabiankaegy
Checklist: