-
Notifications
You must be signed in to change notification settings - Fork 92
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
[KOptionalText] New component #403
[KOptionalText] New component #403
Conversation
3781c25
to
f67f756
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.
Hi @AlexVelezLl code generally LGTM! Please see herein a few recommendation
33417bc
to
d0ec826
Compare
Hi @akolson! Thank you for the recommendations! I have updated the branch with them :) |
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.
Thanks @AlexVelezLl for the changes. LGTM!
Once the accompanying PR in Kolibri/Studio(if any) has been tested for functionality of the component, this PR should be merged! cc @marcellamaki
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.
I think it would be good to add this new component to the documentation, especially as you have already written out examples. See here on how to do this: https://github.com/learningequality/kolibri-design-system#development-of-the-documentation-site
The other slight concern is that the Kolibri frontend tests for this integration are failing - showing empty string instead of the placeholder text. Possible that this is a result of stubbing of child components, but would be good to verify what is happening there.
d0ec826
to
9e16ec8
Compare
Hi @rtibbles! tests on Kolibri were fixed, and docs for this component were added! Let me know if everything is ok with the wording of the docs. Once this PR is merged, I can proceed with updating learningequality/kolibri#10050 to bring the reference back to the main KDS branch. |
</tbody> | ||
</table> | ||
</DocsShow> | ||
</DocsPageSection> |
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.
Could you add a second example showing how it works with the default slot as well?
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.
Sure! Could it be a new row on the current example table? Or should it be a new different example?
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.
I think maybe do it as a separate example, and then you can add some explanatory text about how it is different to the example above. Can even show the difference in a codeblock like you did in your PR description. For an example of showing code instead of rendering the code, see here: https://github.com/learningequality/kolibri-design-system/blob/develop/docs/pages/kcheckbox.vue#L67
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.
Got it! I have added one more example, with code examples 😃
Description
Add a new component to KDS
KOptionalText
that will either display the text orKEmptyPlacehlder
if it's an empty string, undefined, or null.Issue addressed
Addresses #285
Steps to test
KOptionalText
component as described in Implementation notes.Implementation notes
At a high level, how did you implement this?
KOptionalText
will recieve atext
prop or slot that renders text nodes, if there is text to show, the text or the slot will be shown whithin aspan
element, if not, theKEmptyPlacehlder
component will be shown. Also atextStyle
prop can be passed which will be set to thespan
element.Examples:
All of them will render "Hello world" in red color:
If
name
andlastname
are nonempty strings name and lastname will be shown. If not it will render a "-":Testing checklist
changelog
Reviewer guidance