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

A new documentation page and layout fixes for KBreadcrumbs #292

Merged
merged 8 commits into from
Jan 24, 2022

Conversation

MisRob
Copy link
Member

@MisRob MisRob commented Dec 31, 2021

Description

(1) Adds a first version of KBreadcrumbs page to components documentation

(2) Consolidates styles in regards to vertical alignment of breadcrumbs items

Before After
KDS docs doc-breadcrumbs-vertical-align-before doc-breadcrumbs-vertical-align-after

(3) Optimizes breadcrumbs to use all space available

Before After
KDS docs doc-breadcrumbs-space-before doc-breadcrumbs-space-after
Kolibri kolibri-breadcrumbs-before kolibri-breadcrumbs-after

(4) Consolidates throttling and resizing logic

Particularly in regards to having more breadcrumbs instances on one page like in KDS documentation. See commit messages and code comments for details. In the following recording, notice the second breadcrumbs not being properly collapsed because updateCrumbs never ran for this particular instance:

Before After
KDS docs doc-breadcrumbs-throttle-before doc-breadcrumbs-throttle-after

Issue addressed

Steps to test

The documentation page

Regarding text, this is just the very first draft that’s main purpose was to provide me with testing and development environment for new updates as @indirectlylit suggested:

I realized that a simple alternative to Story Book integration is to simply expand the number of examples we embed in the K* pages. In this case for example I was able to update and test the changes without ever leaving the docs

Feel free to suggest any changes. @jtamiace I’d appreciate particularly your thoughts. Also, @jtamiace, I’d like to add some guidance to the new page on when to allow displaying a single breadcrumb item (#129) on this opportunity. Do you have any suggestions?

The library component

Testing checklist

  • Contributor has fully tested the PR manually
  • If there are any front-end changes, before/after screenshots are included
  • The change has been added to the changelog

Reviewer guidance

  • Is the code clean and well-commented?
  • Are all UI components LTR and RTL compliant (if applicable)?

Post-merge updates and tracking

  • Learning Platform update PR is submitted
  • Learning Platform update PR is merged
  • Studio update PR is submitted
  • Studio update PR is merged
  • Data Portal update PR is submitted
  • Data Portal update PR is merged

Comments

I’ll update the changelog and create a new release tag as soon as the first review of the documentation page and the component is done.

Copy link
Member

@nucleogenesis nucleogenesis left a comment

Choose a reason for hiding this comment

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

Code looks good. I love the documentation. Since this is documented directly on the component page, I wonder if we might consider a design that shows code example along with the example. Looking at the code for the docs component I think it'd be nice to see directly in the docs. (I'll tuck this thought away for an upcoming tactical so no need to do anything here). I didn't spin it up because it looks good in your screenshots and on the live netlify of the docs.

Comment on lines +196 to +198
// The throttled update function is defined here and not as a method on purpose
// since having it defined as a method on the options object would cause problems
// when there are more KBreadcrumbs instances rendered on one page.
Copy link
Member

Choose a reason for hiding this comment

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

This makes sense thanks for the explanation!

@MisRob
Copy link
Member Author

MisRob commented Jan 7, 2022

@nucleogenesis

wonder if we might consider a design that shows code example along with the example

Yes, I was thinking about exactly the same thing. I welcome chatting about it during the tactical as you suggest since it's related rather to the whole KDS documentation.

@jtamiace
Copy link
Member

jtamiace commented Jan 7, 2022

I’d like to add some guidance to the new page on when to allow displaying a single breadcrumb item (#129) on this opportunity.

The only situation I can think of that would be useful to show the single breadcrumb is when there isn't any other page header or label that shows what the current level is.

@indirectlylit
Copy link
Contributor

wonder if we might consider a design that shows code example along with the example

Hi friends! Thought I'd provide a little context here:

The DocsShowCode component can be useful. The new layout page has some instances of showing code + examples.

However, the component does not actually render the example code – that is a separate block.

This is somewhat unfortunate because the code is duplicated. On the other hand, it can be useful because some examples need additional code (e.g. css margins) that is irrelevant to demonstrating the component API and this can be omitted.

Another option and something that could be useful regardless: changing the Github link into a "View source" link for the current page, which would let devs more easily inspect the source directly kind of like on readthedocs:

kds rtd
image image

Also some history:

At one point we actually had a system working where the code displayed would drive a live example, and you could even edit the code to see changes. This ended up being challenging because it required the Vue template compiler to be loaded client-side and it required the entire template to shown including the entire <template /> and <script /> tags which was distracting. If these challenges can be overcome that could be neat, but I personally gave up on it in favor of the simpler DocsShowCode.

@MisRob
Copy link
Member Author

MisRob commented Jan 11, 2022

Thank you, @indirectlylit.

@jtamiace I added your suggestion on allowing a single breadcrumb to the guidelines section. Could you please review the whole page?

@MisRob
Copy link
Member Author

MisRob commented Jan 11, 2022

Copy link
Member

@jtamiace jtamiace left a comment

Choose a reason for hiding this comment

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

Thank you, this new documentation is great

@MisRob MisRob marked this pull request as ready for review January 24, 2022 15:23
@marcellamaki marcellamaki merged commit c829d49 into learningequality:main Jan 24, 2022
@MisRob MisRob deleted the kbreadcrumbs branch January 25, 2022 13:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
TODO: needs review Waiting for review
Projects
None yet
5 participants