Skip to content
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

KCardGrid part 2: Add KCardGrid with the base layouts + Add selection controls + Final documentation for KCard and KCardGrid + Few KCard bugfixes and improvements #785

Open
wants to merge 29 commits into
base: develop
Choose a base branch
from

Conversation

MisRob
Copy link
Member

@MisRob MisRob commented Sep 17, 2024

Description

KCard bugfixes, visual and public API updates

  • Adds support for selection controls such as checkboxes
  • Updates related to spaces between sections:
    • Aligns spaces more closely to designs
    • Introduces more robust spacing technique to ensures spaces are rendered as expected no matter of whether some/all/none slots are displayed
    • Improves calculations coordinating space taken by the thumbnail and other content in the horizontal orientation so that no free space is wasted (progressive update taking effect in browsers that support clamp())
Before After
Screenshot from 2024-09-17 20-57-49 Screenshot from 2024-09-17 20-58-57
  • Hides the placeholder after the thumbnail image is loaded
  • Renames titleLines prop to titleMaxLines
  • Renames layout prop to orientation

KCardGrid

  • Introduces KCardGrid with the base layouts
  • KCard updates to make screen readers announce only card titles when navigating the grid with TAB key

Docs

  • Organizes and adds more guidance for KCard docs
  • Adds new KCardGrid docs
  • New documentation components to help with organizing larger amounts of guidance: DocsSubNav, DocsToggleButton, DocsToggleContent, DocsTable
  • Slightly increases the width of the main documentation area to better demonstrate grids

Also has some minor cleanup.

See details and reasons for changes in commit messages.

References

Changelog

TODO @MisRob

  • Description: Summary of change(s)
  • Products impact: Choose from - none (for internal updates) / bugfix / new API / updated API / removed API. If it's 'none', use "-" for all items below to indicate they are not relevant.
  • Addresses: Link(s) to GH issue(s) addressed. Include KDS links as well as links to related issues in a consumer product repository too.
  • Components: Affected public KDS component. Do not include internal sub-components or documentation components.
  • Breaking: Will this change break something in a consumer? Choose from: yes / no
  • Impacts a11y: Does this change improve a11y or adds new features that can be used to improve it? Choose from: yes / no
  • Guidance: Why and how to introduce this update to a consumer? Required for breaking changes, appreciated for changes with a11y impact, and welcomed for non-breaking changes when relevant.

Steps to test

  • Read both documentation pages for KCard and KCardGrid

    • Is all guidance clear?
    • Is there some missing guidance?
  • Test live examples on both documentation pages

    • Preview resulting markup
    • Try keyboard navigation
    • Try screen reader output

I can also provide a copy of the playground page with all visual layouts. However now when there's many live examples on documentation pages, for the purpose of review it would make more sense to use them as they offer larger variety regarding card content.

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 is described in the changelog section above

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

  • This PR completes all main pieces currently needed in Kolibri and Studio. After it's merged, we can close [BETA] Card grid and checkboxes #748 and link to KDS latest release.
  • Next PRs in the series will introduce useKCardGrid for advanced grid customization and loading states

to have a suitable location
for playground sub-components
when testing complex scenarios
alongside the existing very small picture
to allow for testing in both cases.
to visually separate it from the footer slot.
to allow components that required more space
to demonstrate their function in relevant ways,
such as grids.
Ensures spaces are rendered as expected
no matter of whether some/all/none slots
are displayed. Also aligns spaces more closely
to designs.
and use constants to define thumbnail width
so they can be later referenced instead
of hardcoded values..
the content area next to it more intelligently
in browsers that support `clamp()` by:

 - Instead of defining 'width', 'min-width', and 'max-width' separately,
   `clamp()` is used with the goal to have the actual thumbnail width
   saved in the single `$thumbnail-width` value.

- The `$thumbnail-width` value is then referenced when calculating
  the remaining space for the content area, ensuring the precise
  distribution of space.

Resolves some issues related to unprecise calculations, most importantly
this removes the area of empty space between the thumbnail and content areas
in some card's sizes, wasting space that can be used for card's textual content.
Otherwise in some scenarios, such as when there is a large
placeholder element and a small thumbnail image, some parts
of the placeholder element may be visible behind the image
after it has been successfully loaded.
to communicate its function more clearly
to communicate its purpose.

The card layout is rather a combination
of its orientation and the thumbnail area
configuration.
when navigating the list of cards with TAB key.

This resolves overwhelming output and aligns the screenreader
navigation to the expected experience when on TAB only card titles
should be announced (other content is still accessible by  navigating
with arrows).

The main culprit that caused all content of cards being announced
was 'tabindex=0' set on <li>. To resolve the problem,
an alternative technique that doesn't require this tabindex is used
to make the focus ring display around the whole card area.
by screenreaders and add some docs.
This information, with more, is now documented
in the main documentaton sections.
@MisRob MisRob changed the title KCardGrid part 2: Add KCardGrid with the base layouts + Final documentation for KCard and KCardGrid + Few KCard bugfixes KCardGrid part 2: Add KCardGrid with the base layouts + Add selection controls + Final documentation for KCard and KCardGrid + Few KCard bugfixes and improvements Sep 17, 2024
@MisRob MisRob marked this pull request as ready for review September 17, 2024 19:15
@MisRob
Copy link
Member Author

MisRob commented Sep 17, 2024

@radina Regarding checkboxes toggling working only with SPACE but not ENTER, I noticed ENTER is not working on our KCheckboxes in general. So if that was expected experience, we will need to fix, just not in the scope of this work. Will wait for your confirmation and can open issue.

@MisRob
Copy link
Member Author

MisRob commented Sep 17, 2024

I'd like to ask

Note that even though the documentation build shows as failed in the PR checks, I was able to re-run it. Just use the links to the docs in the PR description. Fixed

Thanks all :)

@AllanOXDi
Copy link
Member

@AllanOXDi @AlexVelezLl - would you review code?
I will review it. Thanks

@MisRob
Copy link
Member Author

MisRob commented Sep 18, 2024

Thanks @AllanOXDi :)

For documentation pages code, I think there's no need to spend much time on it - it's large diff and if something went so wrong, we'd spot it on the generated docs. cc @AlexVelezLl

I think in one of the upcoming PRs, I'd like to move live examples to sub-components to make further updates of documentation simpler. Relatedly, I'd like to revive @AlexVelezLl's idea about toggling code. Perhaps something we can begin experimenting with and discuss as a potential pattern. Let's chat in meetings :)

@AllanOXDi
Copy link
Member

Hi @MisRob I started reviewing this PR . Overall, the component is very robust and thoughtfully designed for flexibility and reusability. I have posted few comments for optimizations around code reuse, style handling, and event management. Thank you so much for your great work here.

* Sets card title if provided. The title should be
* unique within a grid and descriptive to aid screenreader
* navigation.
* Card title
*/
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since title prop is set to null by default. We might want to add an additional check to ensure that either the title prop or the title slot is always present since this is critical for a for KCard .

},
headingElement() {
return this.headingLevel ? 'h' + this.headingLevel : undefined;
},
layoutClass() {
return this.stylesAndClasses.layoutClass;
cardAreaClasses() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To me it is appearing that there are minor redundancies in cardAreaClasses and thumbnailStyles. It might be worth refactoring these into helper functions to reduce code duplication, especially for the various combinations of orientation and thumbnail sizes?

Comment on lines +31 to +46
setup(props) {
const { currentLevelConfig } = useResponsiveGridLayout(props);
const gridStyle = ref({});
const gridItemStyle = ref({});
watch(
currentLevelConfig,
newValue => {
const { cardsPerRow, columnGap, rowGap } = newValue;
gridStyle.value = {
'column-gap': columnGap,
'row-gap': rowGap,
};
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be helpful to include more detailed comments here to make it easier for others like me

name: 'KCardGrid',
setup(props) {
const { currentLevelConfig } = useResponsiveGridLayout(props);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think let's destructure props to make it clearer which properties are being used.

@@ -150,19 +167,25 @@
export default {
name: 'KCard',
setup() {
// provided by `KCardGrid`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we change this comments to //gridItemStyle controls the width and layout of KCard items in the grid. either from here on KCardGrid


<p>Grid's visual behavior is based on the <DocsInternalLink text="window breakpoint system" href="/layout#responsiveness" />. <code>KCardGrid</code> determines how many cards per row to display based on its layout and a current window breakpoint.</p>

<p><code>KCardGrid</code> doesn't manage inner card content. This is <code>KCardGrid</code>'s responsibility.</p>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Contradictory Statement here. It might be clearer to state that KCardGrid does not manage the content but only the layout of the cards.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants