Made the KLogo component #460

ShivangRawat30 · 2023-10-01T19:57:23Z

Description

This PR Adds KLogo

Issue addressed

Addresses #373

Steps to test

Navigate to http://localhost:4000/klogo

(optional) Implementation notes

At a high level, how did you implement this?

Does this introduce any tech-debt items?

Testing checklist

Contributor has fully tested the PR manually
If there are any front-end changes, before/after screenshots are included
Critical and brittle code paths are covered by unit tests
The change has been added to the changelog

Reviewer guidance

Is the code clean and well-commented?
Are there tests for this change?
Are all UI components LTR and RTL compliant (if applicable)?
Add other things to check for here

Comments

Signed-off-by: shivangrawat30 <[email protected]>

nucleogenesis

I'll leave approval to @thanksameeelian to be sure here, but I do question whether or not this should be a prop in this component or if we should instead make a separate KLogo or something which uses KImg without these changes rather than making the otherwise generic KImg component configurable for one particular special purpose by a prop.

nucleogenesis · 2023-10-01T23:06:41Z

lib/KImg.vue

@@ -14,6 +14,8 @@

 <script>

+  import kolibriLogo from '../docs/common/DocsPageTemplate/SideNav/kolibri-logo.svg';


I'm not sure if we should be linking to an SVG in the docs directory here. Perhaps it'd be worth copy-pasting it directly into the same directory as this file?

Definitely would be good to copy it to KImg/KLogo directory

I doubt that someone re-organizing docs code and pages would get an idea to check whether it broke something in library. So good to have strict separation.

nucleogenesis · 2023-10-01T23:07:48Z

lib/KImg.vue

@@ -115,6 +124,9 @@
      imgMinWidth() {
        return this.validateAndFormatUnits(this.minWidth);
      },
+      logo() {


Since the item being shown won't always be a "logo" maybe we can call this something like imgSrc

MisRob · 2023-10-02T08:34:29Z

@nucleogenesis

but I do question whether or not this should be a prop in this component or if we should instead make a separate KLogo or something which uses KImg without these changes rather than making the otherwise generic KImg component configurable for one particular special purpose by a prop

It was just a light-hearted issue we made with @akolson to conclude his project in a little silly way utilizing our LE slogan "Kolibri Fly!" :) I admit I didn't put much technical thinking into it. This PR follows the specification exactly #373, so the fault is on me.

You're right that KLogo would make more sense. So, perhaps we could

If @ShivangRawat30 wants to refactor it, you're welcome, but please know we realize that the problem is the specification and not your work on it
If you can't to make these updates @ShivangRawat30, I would still vote for accepting the PR @nucleogenesis if it passes the review and then I would make sure to address this concern myself soon in a follow-up.

Would that be okay?

ShivangRawat30 · 2023-10-02T11:56:05Z

@MisRob I would appreciate the opportunity to improve it. Should I create a new file named Klogo.vue and create a separate tag for the logo image?

MisRob · 2023-10-02T12:42:00Z

Great, @ShivangRawat30, thanks for being open. Yes, it'd be a new component named KLogo.vue. You can find some guidance on creating a new component in the dev docs (https://github.com/learningequality/kolibri-design-system/blob/release-v1.5.x/dev_docs/03_how_to_update_library.md#addupdate-a-component). Optionally, you could create a new simple documentation page for it if you'd like to give it a try https://github.com/learningequality/kolibri-design-system/blob/release-v1.5.x/dev_docs/04_how_to_update_docs.md#create-a-documentation-page-for-a-new-component but we can also open a follow-up issue. It's up to you.

@nucleogenesis Could you please provide some guidance to @ShivangRawat30 if he has some more questions?

ShivangRawat30 · 2023-10-02T17:44:45Z

@MisRob @nucleogenesis Should I create a new folder specifically for images to house the kolibrilogo.svg, or is it more appropriate to retain it within the lib folder?

thanksameeelian · 2023-10-03T18:14:35Z

@MisRob @nucleogenesis Should I create a new folder specifically for images to house the kolibrilogo.svg, or is it more appropriate to retain it within the lib folder?

@ShivangRawat30 you can make your new KLogo component a directory that houses an index.vue and the svg file, like the file structure used in this example from Kolibri. this is mostly a question of preference about organization rather than about function, but it would be nice to uphold that same pattern 🙂

Signed-off-by: shivangrawat30 <[email protected]>

…n-system into 373

Signed-off-by: shivangrawat30 <[email protected]>

ShivangRawat30 · 2023-10-03T18:34:11Z

@nucleogenesis Please review the PR with the changes I've implemented.

ShivangRawat30 · 2023-10-03T18:34:53Z

screenshot of the implemented change.

MisRob · 2023-10-06T07:35:10Z

Wonderful @ShivangRawat30, and thanks for drafting the documentation page! I will leave the final review of this work on @thanksameeelian or @nucleogenesis. For now, only one high-level request - instead of <img>, could you use <KImg>? Then, we wouldn't need to have the same logic two times.

You can just pass KLogo props down to KImg, for example

<div>
  <KImg
    :height="height"
    :width="width"
    ...
  />
</div>

lib/KLogo/index.vue

MisRob · 2023-10-06T07:38:19Z

Also, @radinamatic, when displaying our logo https://raw.githubusercontent.com/learningequality/kolibri-design-system/ff15d1a2d8a3ae82c61de82ede74caf8177a1c9d/lib/KLogo/kolibri-logo.svg, does it need to have alternative text or should it be empty?

radinamatic · 2023-10-06T11:50:44Z

@MisRob Yes, alternative text for the logo image would be required.

If the logo is used just as a flat image, the string Kolibri logo should suffice. If, however, the logo image is also a link, than the alternative text should give the context where the link is leading (for example Go to home page, or similar).

MisRob · 2023-10-06T11:56:23Z

@radinamatic Thank you.

@ShivangRawat30 based on @radinamatic's comment, I'd then suggest different API for KLogo than for KImg in regards to alternative text:

no isDecorative prop (as logo will never be decorative)
altText prop will stay (with no default value though as we may need to translate it, and this won't happen in KDS) and it will be required prop

You will then just send KLogo's altText to KImges altText

Signed-off-by: shivangrawat30 <[email protected]>

ShivangRawat30 · 2023-10-11T08:19:06Z

@MisRob I've made the suggested changes, please review it.

MisRob · 2023-10-12T13:09:01Z

Please note that release-v1.5.x branch was renamed to main

MisRob

Thank you, @ShivangRawat30, this is starting to look great.

I added one note related to removing the logic that's duplicate of KImg'es logic

lib/KLogo/index.vue

Signed-off-by: shivangrawat30 <[email protected]>

ShivangRawat30 · 2023-10-14T08:19:40Z

@MisRob Done!!

MisRob

Almost there @ShivangRawat30. Left you one comment. In KLogo, there is no need for any duplicate of KImg logic. We only need to keep things specific particular to KLogo there (e.g. different alt text requirements)

lib/KLogo/index.vue

Signed-off-by: shivangrawat30 <[email protected]>

MisRob

@ShivangRawat30 Thanks for all work here!

I pushed a commit to finalize the documentation page - finishing docs is something we don't require from contributors, so I organized it myself rather than writing review. It was primarily adjustments related to organization and conventions we use. Your draft was very helpful and saved us time, thank you.

On that opportunity I also pushed a small cleanup to KLogo itself, primarily using required instead of throwing a custom warning (it's a different situation from KImg's warning).

You're welcome to ask any questions and I can elaborate more if there's something you're interested in.

I will update changelog next week and merge.

MisRob · 2023-10-20T13:32:15Z

@radinamatic I used some of your alt text notes for the usage section in the docs

radinamatic · 2023-10-20T14:40:15Z

docs/pages/klogo.vue

+    <DocsPageSection title="Usage" anchor="#usage">
+      <h3>Alternative text</h3>
+
+      <p>Alternative text (<code>altText</code>) is always required for the logo image. When creating it, consider the following:</p>


Looking good @MisRob, I'd just suggest to remove 'always', as in the wide world logos are not absolutely required to have alternative text. They can also be used for decorative purposes, but I'm unaware we are using ours that way anywhere in Kolibri. In our case just 'required' should suffice.

Alright, will do before merging. Thank you

ShivangRawat30 · 2023-10-20T16:44:05Z

@MisRob I greatly appreciate your help related to the PR, and I'm eager to make further contributions to the codebase. Please consider assigning me more issues as I'm enthusiastic about resolving them.

MisRob · 2023-10-23T08:30:15Z

You're welcome, @ShivangRawat30! We welcome contributions, and I think there are some free "help wanted" or "good first issue" issues here in this repository or in Kolibri or Studio. Let us know if you've found something suitable.

Made the kolibriFly prop

03338bb

Signed-off-by: shivangrawat30 <[email protected]>

nucleogenesis requested a review from thanksameeelian October 1, 2023 23:03

nucleogenesis added the TODO: needs review Waiting for review label Oct 1, 2023

nucleogenesis reviewed Oct 1, 2023

View reviewed changes

ShivangRawat30 and others added 4 commits October 3, 2023 23:57

KLogo component made

9c067f5

Signed-off-by: shivangrawat30 <[email protected]>

Merge branch '373' of https://github.com/ShivangRawat30/kolibri-desig…

c2311ae

…n-system into 373

minor change

1562e9a

Signed-off-by: shivangrawat30 <[email protected]>

Rename Index.vue to index.vue

ff15d1a

MisRob changed the title ~~Made the kolibriFly prop~~ Made the KLogo component Oct 4, 2023

MisRob reviewed Oct 6, 2023

View reviewed changes

lib/KLogo/index.vue Outdated Show resolved Hide resolved

isDecorative removed.

b5774e0

Signed-off-by: shivangrawat30 <[email protected]>

MisRob requested changes Oct 12, 2023

View reviewed changes

lib/KLogo/index.vue Outdated Show resolved Hide resolved

ShivangRawat30 and others added 3 commits October 14, 2023 13:29

Update index.vue

af6bc07

Update index.vue

faf3f1a

minor change

36321df

Signed-off-by: shivangrawat30 <[email protected]>

MisRob requested changes Oct 16, 2023

View reviewed changes

lib/KLogo/index.vue Outdated Show resolved Hide resolved

ShivangRawat30 added 5 commits October 19, 2023 19:03

changes KLogo props

b07e362

Signed-off-by: shivangrawat30 <[email protected]>

minor changes

dd94eeb

Signed-off-by: shivangrawat30 <[email protected]>

removed validateAndFormatUnits

b4d4f46

Signed-off-by: shivangrawat30 <[email protected]>

commit

b198aba

Signed-off-by: shivangrawat30 <[email protected]>

linting fixed

9f4c6f3

Signed-off-by: shivangrawat30 <[email protected]>

MisRob mentioned this pull request Oct 20, 2023

KImg: Kolibri Fly! #373

Closed

2 tasks

MisRob self-requested a review October 20, 2023 12:37

MisRob added 4 commits October 20, 2023 14:42

Cleanup KLogo

db2d355

Use lowercase for docs page name

585985f

Finalize KLogo docs page

56a795a

Merge branch 'main' into 373

6e3dee1

MisRob approved these changes Oct 20, 2023

View reviewed changes

radinamatic reviewed Oct 20, 2023

View reviewed changes

MisRob added 3 commits October 23, 2023 12:07

Improve guidance wording

f00125e

Merge branch 'main' into 373

6b43d8b

Update changelog

945c281

MisRob force-pushed the 373 branch from b0077aa to 945c281 Compare October 23, 2023 10:18

MisRob merged commit 8a889bd into learningequality:main Oct 23, 2023
7 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Made the KLogo component #460

Made the KLogo component #460

ShivangRawat30 commented Oct 1, 2023 •

edited by MisRob

Loading

nucleogenesis left a comment

nucleogenesis Oct 1, 2023

MisRob Oct 2, 2023

MisRob Oct 2, 2023

nucleogenesis Oct 1, 2023

MisRob commented Oct 2, 2023 •

edited

Loading

ShivangRawat30 commented Oct 2, 2023

MisRob commented Oct 2, 2023

ShivangRawat30 commented Oct 2, 2023

thanksameeelian commented Oct 3, 2023

ShivangRawat30 commented Oct 3, 2023

ShivangRawat30 commented Oct 3, 2023

MisRob commented Oct 6, 2023

MisRob commented Oct 6, 2023

radinamatic commented Oct 6, 2023

MisRob commented Oct 6, 2023 •

edited

Loading

ShivangRawat30 commented Oct 11, 2023

MisRob commented Oct 12, 2023

MisRob left a comment

ShivangRawat30 commented Oct 14, 2023

MisRob left a comment

MisRob left a comment

MisRob commented Oct 20, 2023

radinamatic Oct 20, 2023 •

edited

Loading

MisRob Oct 23, 2023

ShivangRawat30 commented Oct 20, 2023

MisRob commented Oct 23, 2023

		@@ -14,6 +14,8 @@

		<script>

		import kolibriLogo from '../docs/common/DocsPageTemplate/SideNav/kolibri-logo.svg';

Made the KLogo component #460

Made the KLogo component #460

Conversation

ShivangRawat30 commented Oct 1, 2023 • edited by MisRob Loading

Description

Issue addressed

Steps to test

(optional) Implementation notes

At a high level, how did you implement this?

Does this introduce any tech-debt items?

Testing checklist

Reviewer guidance

Comments

nucleogenesis left a comment

Choose a reason for hiding this comment

nucleogenesis Oct 1, 2023

Choose a reason for hiding this comment

MisRob Oct 2, 2023

Choose a reason for hiding this comment

MisRob Oct 2, 2023

Choose a reason for hiding this comment

nucleogenesis Oct 1, 2023

Choose a reason for hiding this comment

MisRob commented Oct 2, 2023 • edited Loading

ShivangRawat30 commented Oct 2, 2023

MisRob commented Oct 2, 2023

ShivangRawat30 commented Oct 2, 2023

thanksameeelian commented Oct 3, 2023

ShivangRawat30 commented Oct 3, 2023

ShivangRawat30 commented Oct 3, 2023

MisRob commented Oct 6, 2023

MisRob commented Oct 6, 2023

radinamatic commented Oct 6, 2023

MisRob commented Oct 6, 2023 • edited Loading

ShivangRawat30 commented Oct 11, 2023

MisRob commented Oct 12, 2023

MisRob left a comment

Choose a reason for hiding this comment

ShivangRawat30 commented Oct 14, 2023

MisRob left a comment

Choose a reason for hiding this comment

MisRob left a comment

Choose a reason for hiding this comment

MisRob commented Oct 20, 2023

radinamatic Oct 20, 2023 • edited Loading

Choose a reason for hiding this comment

MisRob Oct 23, 2023

Choose a reason for hiding this comment

ShivangRawat30 commented Oct 20, 2023

MisRob commented Oct 23, 2023

ShivangRawat30 commented Oct 1, 2023 •

edited by MisRob

Loading

MisRob commented Oct 2, 2023 •

edited

Loading

MisRob commented Oct 6, 2023 •

edited

Loading

radinamatic Oct 20, 2023 •

edited

Loading