kolibri-design-system icon indicating copy to clipboard operation
kolibri-design-system copied to clipboard

Issue 921

Open yeshwanth235 opened this issue 7 months ago • 4 comments

Description

Added a Usage section to the KModal documentation.

Issue addressed

#921

Before/after screenshots

Before image

After image

Changelog

  • Description: Added a Usage section to the KModal documentation. Usage section items 1. Dialog Box with Description 2. Submit Form Dialog Box 3. Modal with Different Sizes
  • Products impact: None
  • Addresses: #921
  • Components: - None
  • Breaking: No
  • Impacts a11y: No
  • Guidance: None

Steps to test

  1. Step 1
  2. Step 2
  3. ...

(optional) Implementation notes

At a high level, how did you implement this?

Does this introduce any tech-debt items?

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

It has a dependency on the PR below, so we need to merge that one first. PR: https://github.com/learningequality/kolibri-design-system/pull/1021

yeshwanth235 avatar May 13 '25 15:05 yeshwanth235

Hi @yeshwanth235, thanks for the contribution! This will be helpful.

  • There are some unrelated files, such as kcard and DocsExample. Seems it just leaked from another task? In this branch, you will need to keep only changes related to the documentation page you intend to update.
  • In the screenshot, I see titles of sections are capitalized. Please use "Sentence case" capitalization for titles, not "Title Case" capitalization, and on that opportunity, I'd like to ask you to have a look these Writing guidelines. After you feel it is aligned, let me know and I will review in detail.

MisRob avatar May 21 '25 02:05 MisRob

Hi @MisRob, I agree with the first point. I am dependent on [#1021]. Once it is merge. Will Update the PR.

yeshwanth235 avatar May 21 '25 03:05 yeshwanth235

@yeshwanth235 You can remove chosen files from a pull request (= revert to develop versions) with git checkout develop -- <file_path> and then commit. That said, this is not a pressing task, and no problem to wait. I'm suggesting this in case you didn't know about this command and perhaps want to learn something new.

And for next time, if you pick up another issue, it is possible and common to open multiple pull requests at once for various tasks - by starting a new branch from develop for each task rather than from your another working branch. And if this happens by mistake, you can look into rebasing.

MisRob avatar May 21 '25 04:05 MisRob

To make it clear. Since i needed hideStyle prop. I have created my branch from existing issue. Once we merge that PR. I will take pull from develop. It will resolve the issue and only show issue 921 changes. I hope you get it 🙂 @MisRob

yeshwanth235 avatar May 21 '25 06:05 yeshwanth235

Thanks for clarification, I assume you mean this place:

<DocsExample
  loadExample="KModal/ModalWithDifferentSizes.vue"
  :hideStyle="true"
 />

I haven't noticed before. Yes then, it's best to wait :+1:

MisRob avatar May 21 '25 07:05 MisRob

Hi @MisRob Updated the PR. Please help to review

yeshwanth235 avatar May 22 '25 03:05 yeshwanth235

Percy Visual Test Results

Percy Dashboard: View Detailed Report

Environment:

  • Node.js Version: 18.x
  • OS: Ubuntu-latest

Instructions for Reviewers:

  • Click on the Percy Dashboard link to view detailed visual diffs.
  • Review the visual changes highlighted in the report.
  • Approve or request changes based on the visual differences.

github-actions[bot] avatar May 22 '25 03:05 github-actions[bot]

Great, I will have a look @yeshwanth235

MisRob avatar May 22 '25 04:05 MisRob

It's possible that those previous unresolved comments were hidden under 'Load more':

Screenshot from 2025-05-29 08-12-17

MisRob avatar May 29 '25 06:05 MisRob