Initial implementation of KImg component #404
Conversation
|
@jtamiace if you'd like to read through the documentation, we'd appreciate your input |
|
Documentation preview https://deploy-preview-404--kolibri-design-system.netlify.app/kimg/ |
| new Page({ | ||
| path: '/kimg', | ||
| title: 'KImg', | ||
| isCode: true, |
There was a problem hiding this comment.
It would be nice to add the keywords property to allow for better searching within the documentation. For example, currently when you try to search an image component using the "image" keyword, the search would yield no results. Please see usages within this file for implementation details
There was a problem hiding this comment.
this isn't automatically showing up as outdated since i added code on the line below, but after checking the code please let me know whether you think the additions in my most recent commit are sufficient or you'd like to see additional keywords.
There was a problem hiding this comment.
I think the changes made should suffice. Thanks @thanksameeelian. Non-blocking but would be good to follow existing patterns i.e. pass variable to keywords as opposed to the actual array object
There was a problem hiding this comment.
thanks! i checked out that pattern when i was adding the keywords here, but only saw the pattern of using a variable when multiple items had the same keywords, so the variable was being used in at least two places.
i don't think that pattern is applicable here, as KImg is the only item using those specific keywords. i am following the pattern i saw for each item that had a unique list of variables, as in those cases the array itself was passed. you can see this patten in KSelect and KSwitch in the table of contents. i think this makes sense because it avoids unnecessary abstraction.
but if another item were to share the same keywords at kimg, i would totally agree that a variable would be the right call!
lib/KImg.vue
Outdated
| return `${propValue}px`; | ||
| } else { | ||
| // split numbers apart from units | ||
| const [, ...arr] = propValue.match(/(\d*)([\s\S]*)/); |
There was a problem hiding this comment.
@thanksameeelian Awesome approach 💯 :) This was so much fun and exciting to review. It felt like a sprinkling of magic dust!
|
Hi @thanksameeelian I reviewed your code alongside @MisRob and we think your code generally looks good! Great Job :). We found few issues for you to look into. Also, to give us better design perspective especially on the documentation, we've added @jtamiace to give her thoughts on https://deploy-preview-404--kolibri-design-system.netlify.app/kimg/ |
|
Something that we pondered about was whether it was necessary to widen the scope of units used. At the moment, only |
thanks so much @akolson & @MisRob! i've read through your comments & i don't think i have any questions about them, but will be sure to ask if i end up needing clarification. i'll get on these changes but also keep my eyes open for feedback from the design team! |
it was just an oversight that i didn't include a more exhaustive list of units initially, so i'm def happy to update and try to catch 'em all! 😀 |
@akolson @MisRob @thanksameeelian - I think having more units would be great idea! At the same time, in my opinion it isn't necessarily a blocker, unless we can think of a specific use case in one of our products right now where we would likely be needing/using some other unit. If so, that should be prioritized - otherwise I see no problem with expanding this on an as-needed basis. Thanks Amelia for this great work, and Misha and Samson for the very thoughtful and comprehensive review! Very exciting to see this happening :) |
|
This looks great! In terms of additional content, I'm not 100% sure if this suggestion is appropriate for this page which is focused on technical guidance and documentation, but I'm wondering if it makes sense to record any general patterns we use for missing images or error states if an image is broken, or if this is something that's out of the scope of KImg and handled elsewhere. For example in Studio and the Learning Platform we use a light background gray along with a centered icon of the resource type or a generic "image" icon. |
|
One unit I'd suggest adding would be |
… css units, adjust regex, change code sample display
thank you so much for your feedback jessica!! we've split this project up over several prs (here's the epic!) so creating a placeholder image will be handled in the next pr, and additional error states potentially after that. i'd really appreciate your feedback on those once they're ready if you have the time, so i'll be sure to add examples of them in the doc & tag you/request your review! |
|
thanks so much for the initial round of review, @akolson, @MisRob, @marcellamaki, & @jtamiace!! i made all the changes indicated and would love to hear your thoughts on this new version! in terms of CSS units, i added |
There was a problem hiding this comment.
Hi @thanksameeelian, Thanks for the changes, they LGTM! @MisRob @jtamiace, any other thoughts? Also @marcellamaki , at what point do we do manual QA on this? Should we wait for the component to develop further or have QA take a first look?
|
While I think some manual QA would be ideal in most situations, since we don't an immediate github issue in Kolibri that requires this as a fix, and we don't have plans to refactor immediately, I think it's okay to proceed without manual QA for now. There have been a lot of eyes on this, and thoughtful work and feedback, so I feel okay about moving forward without that testing. And, we should remain open to the possibility that some tweaks may be needed as we start integrating this into our products, and that's okay 😄 |
There was a problem hiding this comment.
Thank you @thanksameeelian, good work.
I too feel fine about doing manual QA as soon as the component is complete, as some upcoming issues may perhaps require tweaking code here and there a bit in areas that were implemented before.
Description
This is the initial implementation of the KImg component, which will ultimately fulfill all the specifications of the KImg epic, but for this round only includes
src,altText,isDecorative,height,width,min-width,max-width,min-height, andmax-height.Issue addressed
Addresses #369
Steps to test
yarn installyarn devKImgdoc page should be accessible at http://localhost:4000/kimgTesting checklist
Reviewer guidance
Comments