-
Notifications
You must be signed in to change notification settings - Fork 81
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
Update the kolibri logo. #561
Update the kolibri logo. #561
Conversation
One possibility that occurs is we should include both the logo with a background and without a background here, I'll make the update accordingly. |
da78764
to
d24dd5a
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.
That's all very nice :) Nothing jumped out from the code and the documentation page is very helpful, thank you.
Very minor note is that it may be nice to reformulate a bit this section since it doesn't have much to do with KImg
anymore. Perhaps we could just instead mention that KImg
is for non-logo images.
Sure, that had also occurred to me at one point, and then I'd forgotten about it. I'll update the docs. |
528068c
to
9a29533
Compare
</DocsPageTemplate> | ||
|
||
</template> | ||
|
||
|
||
<script> | ||
|
||
export default {}; | ||
// Note for developers, to utilize the saveSVG method, select all of: |
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.
Handy
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.
Overall it's looking good, thanks for all this work @rtibbles.
Not blocking, just a few doc tweaks that would be nice before merging:
(1) I think we could remove maxSize
from examples code
since it's not a 'meaningful' representation of how the component will be used (with defined size
in px, maxSize
won't have any further effect). If we wanted to demonstrate how to use size
together with maxSize
, something like this would make more sense
but I think it'd be okay to keep examples simple and have just size
.
(2) Relatedly, as we're now using size
language, I think that related props documentation
could benefit from more specificity as to what values are expected.
I wrote some guidance for this in KImg
if it helps (and this reminds me I'd better include it in KImg props docs as well):
Values may be either numbers or strings consisting of a numeral and a valid unit. The following units are supported: %, cm, em, ex, ch, in, lh, mm, px, rem, rlh, vh, vw. If you don't provide a unit, px will be used by default.
Will update, thanks @MisRob! |
…dering. Allow Kolibri logo to be rendered with a background.
9a29533
to
557415e
Compare
Description
Before/after screenshots
All options are documented and demonstrated in the updated documentation.
Changelog
Updated in PR.
Testing checklist
Reviewer guidance
Review the updated KLogo documentation page for this.
Did the minor refactor of KImg put the function in the right place?
After review
CHANGELOG.md
Comments