-
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
Implement BaseCard #548
Implement BaseCard #548
Conversation
lib/KCard/index.vue
Outdated
|
||
<div | ||
class="card" | ||
:class="$computedClass({ ':focus': $coreOutline })" |
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 we have this in the BaseCard
? It's part of the general card logic.
Hi Allan, thank you, seems to go really well :)! I added few random notes. One high-level note, during development I'd suggest adding unit tests for the following:
and perhaps you will locate some other scenarios. There's no need to unit test styles, focus states etc., KDS testing is currently limited rather to business logic and markup. I haven't reviewed in detail areas that seemed to be work in progress. Let me know any time, also if you had any questions. |
lib/KCard/BaseCard.vue
Outdated
<KTextTruncator :text="title" :maxLines="1" /> | ||
</a> | ||
</h2> | ||
<h3 |
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 @AllanOXDi - looks like some great foundational work here! One small suggestion is that you could consider using <component>
here to manage this, rather than write out each condition separately, just to have this be a little bit more concise.
I haven't actually tried to run this code, so it might need some adjustments, but as an example:
<component
:is="'h' + headingLevel"
v-if="title !== null"
>
<a :href="to">
<KTextTruncator :text="title" :maxLines="1" />
</a>
</component>
I think this will work with semantic HTML, and not just custom components that are created within the repository, although there might be some edge cases so it would be good to double check the documentation here and not just take my word for it 😂
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 for the suggestion- I going to be trying it out and see
This has been moved to #625. Closing it |
Description
Issue addressed
Addresses #PR# HERE
Before/after screenshots
Changelog
[#PR no]: PR link
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
After review
CHANGELOG.md
Comments