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

Add documentation pages for KResponsiveWindow and KResponsiveElement #362

Merged
merged 2 commits into from
Sep 21, 2022

Conversation

MisRob
Copy link
Member

@MisRob MisRob commented Sep 19, 2022

Description

Updates library documentation:

  • Updates Layouts page: Some sections related to KResponsiveWindowMixin were moved to its new page, and Layouts page now only contains general breakpoints information with newly added references to the two new pages.
  • Adds two new pages:
    • KResponsiveWindow page: To a large extent, existing documentation from the Layouts page was used.
    • KResponsiveElement page: I wrote down some basic information and usage but didn't create an example since it's quite simple, and also, currently, we rather discourage using it.

Two new pages were prepared with keeping #288 (allow mixins logic to be used via composition API) in mind. Therefore, pages are called KResponsiveWindow/KResponsiveElement and not KResponsiveWindowMixin/KResponsiveElementMixin since in the near future we should be able to access this logic in an alternative way to mixins, via composables (useKResponsiveWindow, useKResponsiveElement). Relatedly, all information provided on new pages is presented in a way applicable to both existing mixins as well as to new composables to be implemented. Usage details specific to mixins can be found in the Usage -> As mixins section of pages. This prepares the ground for a new sub-section, Usage-> As composables to be added as part of #288.

Issue addressed

I mentioned performance troubles on KResponsiveElement page so this also closes #237

Steps to test

Read

Is it easy to follow? Can you spot any typos, recommend grammar improvements?

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

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

@MisRob
Copy link
Member Author

MisRob commented Sep 19, 2022

I'm targeting the release branch because I think it may be useful to have these updates live. After we merge this, I will merge the release branch into the develop branch so it's also available when @akolson starts working on #288 as that work will target the develop branch.

@MisRob MisRob marked this pull request as ready for review September 19, 2022 13:40
@radinamatic
Copy link
Member

Nice worthy additions, docs build well locally, and I did not notice any overt grammar errors:

KResponsive

Given that the KDS documentation is a 👶🏽 of many with varying parenting (writing 🙂) styles that we never had bandwidth to unify from the technical writing point of view, I'm vary of making styling and editing suggestions for these specific components as those will probably be valid for all the rest.

Copy link
Member

@radinamatic radinamatic left a comment

Choose a reason for hiding this comment

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

LGTM @MisRob, thank you!

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.

This is excellent @MisRob - I really love the "change your browser size to see this in action" example you gave :)

@MisRob
Copy link
Member Author

MisRob commented Sep 21, 2022

@nucleogenesis I just moved that example from the Layouts page, and I suspect that @indirectlylit is its author :)

@MisRob MisRob merged commit ecfa5e9 into learningequality:release-v1.4.x Sep 21, 2022
Copy link
Contributor

@indirectlylit indirectlylit left a comment

Choose a reason for hiding this comment

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

looks great!

just a heads up that #358 might start getting a bit stale as the docs evolve

Comment on lines +33 to +39
<DocsShowCode language="javascript">
import KResponsiveWindowMixin from 'kolibri-design-system/lib/KResponsiveWindowMixin';

export default {
mixins: [KResponsiveWindowMixin]
};
</DocsShowCode>
Copy link
Contributor

@indirectlylit indirectlylit Sep 21, 2022

Choose a reason for hiding this comment

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

Comment on lines +53 to +60
<DocsShowCode language="html">
&lt;div class="box" :style="boxStyle"&gt;
Box 1
&lt;/div&gt;
&lt;div class="box" :style="boxStyle"&gt;
Box 2
&lt;/div&gt;
</DocsShowCode>
Copy link
Contributor

Choose a reason for hiding this comment

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

@MisRob
Copy link
Member Author

MisRob commented Sep 21, 2022

Ah I'm sorry @indirectlylit, I knew about that PR. I will assign myself and will do my best to review soon and resolve conflicts.

@MisRob
Copy link
Member Author

MisRob commented Sep 21, 2022

Thanks for pointing out those places

@indirectlylit
Copy link
Contributor

no worries! just wanted to give a heads up - I know there are always a lot of things happening

@MisRob MisRob deleted the document-mixins branch October 14, 2023 11:52
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
Development

Successfully merging this pull request may close these issues.

4 participants