-
Notifications
You must be signed in to change notification settings - Fork 83
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
Complete KImg #538
Complete KImg #538
Conversation
rather than applying them to <img> in preparation for implementing the image scaling logic.
ac800c3
to
77c1423
Compare
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 looks fantastic, @MisRob - the documentation is so clear, and the KImg file itself is also really easy to read. I would love to hear @akolson's thoughts as I know he did some of the speccing with you, so he may have a few additional notes or insights that I don't. But, for me, I don't see any blockers and I really appreciate you pushing on this to get it merged.
@AllanOXDi - no need to formally review here but I think if you read through, and ask any questions you have about the code as you do, this will be some helpful foundational context for the KCard
work that is upcoming, as they are related :)
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.
HI @MisRob! This looks really amazing. The component is so well documented and implemented in the simplest possible way❤️ . No further comments -- I can't wait to see it in action. Thanks
Thanks both, that's kind. We have a co-review planned next week with @AllanOXDi. If all goes well, I will merge after that. |
I'll merge so it's available for components that need it. @AllanOXDi we can still co-review later on when there's more time. |
Sure! I am available for co rearview. |
Description
Completes the implementation of the new
KImg
component.Note there are minor deviations from the original specification, namely:
placeholderColor
prop renamed tobackgroundColor
kolibriFly
prop not implementedKLogo
componentdefault
andplaceholder
slot merged only into one slot,placeholder
#topLeft
,#topRight
,#bottomLeft
, and#bottomRight
slotsKCard
work. I am not entirely sure if they will be needed, because recently I noticed we actually don't place content over card thumbnail images in the new card designs. But since the card designs are WIP and it won't harm anything, I am leaving the slots here for now. They won't be exposed inKCard
if it's not in the designs.Issue addressed
Changelog
Steps to test
Since this work completes
KImg
, please review the component code and documentation page as a whole (plus it'd be quite meaningless to focus on checking the PR diff anyways as I needed to tweak previous code to support new features and we don't really use this component anywhere yet). Read KImg Component #368 and linked issues for expected API.Is the documentation page clear? https://deploy-preview-538--kolibri-design-system.netlify.app/kimg/
Testing checklist
If there are any front-end changes, before/after screenshots are includedReviewer guidance
After review
CHANGELOG.md