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.

Sign up for GitHub

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

Jump to bottom

523: UI updates for the submission steps on "Skill" & "Knowledge" Review forms #581

Merged
merged 15 commits into from
Feb 26, 2025
Merged

523: UI updates for the submission steps on "Skill" & "Knowledge" Review forms #581

merged 15 commits into from
Feb 26, 2025

Conversation

adigidh
Copy link
Contributor

@adigidh adigidh commented Feb 12, 2025 •
edited
Loading

fixes #523

Summary of changes on both skill and knowledge review sections:

  • Update typography across the review submission step (font styles, and sizes) o
  • Added content changes as requested in the figma
  • Updated seed data nested component to display accordion style UX
  • Updated component with semantic HTML

Screen recording ZIP:
instructlab.mov.zip

@Misjohns
Copy link
Collaborator

@adigidh Thank you for picking this up. Just a few tweaks.

The leading (line height) of the samples is too large. Should be 14/21, but looks more like 14/40.
image

The accordion header should match content frame color unless the user is hovering over the header.
Please verify this only displays when the user is hovering over the accordion header.

image

Expanded but no hover
image

Users cursor on accordion header
image

Misjohns
Misjohns requested changes Feb 13, 2025
View reviewed changes
Copy link
Collaborator

@Misjohns Misjohns left a comment

Choose a reason for hiding this comment

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

Left my comments in previous message.

@adigidh
Copy link
Contributor Author

adigidh commented Feb 13, 2025

Thank you for taking a look. Made the following updates @Misjohns :

  1. Improved line height for seed data accordion content

  2. The accordion header should match content frame color unless the user is hovering over the header.
This was actually directly coming from the patteenfly component base styles. Overriden these styles now.
image
  1. Added cursor: pointer for hover on accordion header and content.
hover.mov

@adigidh adigidh changed the title 523: UI updates for the review submission step on "Create Skill Contribution" 523: UI updates for the submission steps on "Skill" & "Knowledge" Review forms Feb 14, 2025
@adigidh adigidh requested a review from Misjohns February 14, 2025 21:38
@adigidh adigidh force-pushed the 523-review-step branch 2 times, most recently from faba118 to 24705d2 Compare February 18, 2025 13:29
@vishnoianil
Copy link
Member

@adigidh Thanks for the PR, I pulled it locally for testing and overall it looks good.

Skill review form looks great!.

In the knowledge review form, When i load Review Submission, browser throws the following error. Looks like you need to use unique key for each list element.

Each child in a list should have a unique "key" prop.
Check the render method of `ReviewSubmission`. See https://react.dev/link/warning-keys for more information.

vishnoianil
vishnoianil reviewed Feb 19, 2025
View reviewed changes
src/components/Contribute/Knowledge/ReviewSubmission/index.tsx Outdated
</p>
<article>
<div className="info-wrapper">
<p className="submission-titles">Author Information</p>
Copy link
Member

Choose a reason for hiding this comment

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

To keep the same format across skill and knowledge review form, can you please update Author Information section to use "Contributor Information/Contributors" and add the relevant context text.

vishnoianil
vishnoianil reviewed Feb 19, 2025
View reviewed changes
src/components/Contribute/Knowledge/ReviewSubmission/index.tsx Outdated
</p>
<article>
{/* File Path Information */}
<h5 className="category-titles">File Path Information</h5>
Copy link
Member

Choose a reason for hiding this comment

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

Let's use Directory Path, similar to skill form.

vishnoianil
vishnoianil requested changes Feb 19, 2025
View reviewed changes
src/components/Contribute/Knowledge/ReviewSubmission/index.tsx Outdated
<p>{knowledgeFormData.knowledgeDocumentRepositoryUrl}</p>
<h5 className="category-titles">Commit</h5>
<p>{knowledgeFormData.knowledgeDocumentCommit}</p>
<h5 className="category-titles">Commit</h5>
Copy link
Member

Choose a reason for hiding this comment

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

typo: It should be Name?

src/components/Contribute/Skill/ReviewSubmission/index.tsx Outdated
<article>
<div className="info-wrapper">
<p className="submission-titles">Contributor Information</p>
<p className="submission-subtitles">Information required for a Github Developer Certificate of Origin (OC) sign-off.</p>
Copy link
Member

Choose a reason for hiding this comment

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

OC -> DCO

src/components/Contribute/Skill/ReviewSubmission/index.tsx Outdated
<>
<section className="review-submission-container">
<Content component={ContentVariants.h3}>Review</Content>
<p>Review the information below and click finish to submit your knowledge contribution. Use the back button to make changes.</p>
Copy link
Member

Choose a reason for hiding this comment

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

This is skill review form, so we need to remove all the reference for "Knowledge" through out this form.

src/components/Contribute/Skill/ReviewSubmission/index.tsx Outdated
{/* Seed Examples */}
<article>
<div className="info-wrapper">
<p>Seed data</p>
Copy link
Member

Choose a reason for hiding this comment

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

Let's use "Seed Example" here.

@adigidh
Copy link
Contributor Author

adigidh commented Feb 20, 2025 •
edited
Loading

Thank you for taking a look, and the detailed review.
Summary of changes: @vishnoianil

  • Resolved duplicated key warning issues. Added unique keys for accordion items and used React.Fragment for unique key for QA pairs.
  • Updated Author Information section to use "Contributor Information/Contributors"
  • Changed File path to “Directory Path”
  • Added fallback check for seed examples data
  • Correct typo per this comment
  • Updated OC -> DCO in content
  • Removed all references for "Knowledge" through out the skill form
  • "Seed Example" used as label text

Can we create a separate tech debt issue to combine these components, add a unit/ integration test plus some documentation ? That will be beneficial to manage this in the future.

@adigidh adigidh requested a review from vishnoianil February 20, 2025 16:27
jeff-phillips-18
jeff-phillips-18 reviewed Feb 24, 2025
View reviewed changes
src/components/Contribute/Knowledge/ReviewSubmission/index.tsx Outdated
{/* Author Information */}
<article>
<div className="info-wrapper">
<p className="submission-titles">Contributor Information</p>
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should use <Content component={ContentVariants.p} instead of direct <p> elements

src/components/Contribute/Knowledge/ReviewSubmission/index.tsx Outdated
</div>

<div className="contributors-wrapper">
<h5 className="category-titles">Contributors</h5>
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should use <Content component={ContentVariants.h5} instead of direct <h5> components.

src/components/Contribute/Skill/ReviewSubmission/index.tsx Outdated
@@ -2,85 +2,107 @@
import { SkillFormData } from '@/types';
import { Content, ContentVariants } from '@patternfly/react-core';
import React from 'react';
import './submission.css';
import { Accordion, AccordionContent, AccordionItem, AccordionToggle } from '@patternfly/react-core';
Copy link
Collaborator

Choose a reason for hiding this comment

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

Add these to the imports from @patternfly/react-core above

src/components/Contribute/Skill/ReviewSubmission/index.tsx Outdated
{/* Author Information */}
<article>
<div className="info-wrapper">
<p className="submission-titles">Contributor Information</p>
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should use the Content components here as well

src/components/Contribute/Skill/ReviewSubmission/submission.css Outdated
.review-submission-container {
.contributors-wrapper {
line-height: 2;
padding-top: 15px;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
padding-top: 15px;
padding-top: var(--pf-t--global--spacer--md);

src/components/Contribute/Skill/ReviewSubmission/submission.css Outdated
}

.accordion-wrapper {
padding-bottom: 15px;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
padding-bottom: 15px;
padding-bottom: var(--pf-t--global--spacer--md);

src/components/Contribute/Skill/ReviewSubmission/submission.css Outdated
}

.category-titles {
font-weight: 600;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
font-weight: 600;
font-weight: var(pf-t--global--font--weight--heading);

src/components/Contribute/Skill/ReviewSubmission/submission.css Outdated
}

.accordion-content {
padding: 15px;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
padding: 15px;
padding: var(--pf-t--global--spacer--md);

src/components/Contribute/Skill/ReviewSubmission/submission.css Outdated
.seed-category-titles {
font-weight: 600;
font-size: var(--pf-t--global--font--size--xs) !important;
padding-bottom: 5px;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
padding-bottom: 5px;
padding-bottom: var(--pf-t--global--spacer--sm);

src/components/Contribute/Skill/ReviewSubmission/submission.css Outdated
}

.seed-category-titles {
font-weight: 600;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
font-weight: 600;
font-weight: var(pf-t--global--font--weight--heading);

@adigidh
Copy link
Contributor Author

adigidh commented Feb 24, 2025

Thank you taking a look @jeff-phillips-18 . Pushing the updates on the PR.

I appreciate the effort in ensuring consistency across components. Will this approach be standardized across all components moving forward? If so, it might be helpful to document this in our contribution guidelines or implement pre-commit or linting checks to enforce these patterns. Example: we can either enforce patternfly usage with ESLINT or create custom ESLint rules, or use pre commit hooks with husky.

That said, I want to flag that the current updates were primarily focused on styling adjustments and adding in the accordion bits, without modifying the underlying elements. Since the existing components in the main branch don’t all follow this pattern of using PatternFly’s components, incorporating such changes could introduce scope creep in issues by adding fixes for technical debt alongside styling updates.

IMO it makes sense to address these tech debt improvements separately to keep the scope focused.

@vishnoianil
Copy link
Member

@adigidh can you please rebase your PR on main now ?

@adigidh
adopting semantic html
eae9e4a 
Signed-off-by: Aditya Gidh <[email protected]>
@adigidh
tweak components, add styles
94fa77f 
Signed-off-by: Aditya Gidh <[email protected]>
@adigidh
getting accordion functional
6e629c7 
Signed-off-by: Aditya Gidh <[email protected]>
@adigidh
update with the accordion set
5edc73d 
Signed-off-by: Aditya Gidh <[email protected]>
@adigidh
adding accordion pieces
ff4c5d6 
Signed-off-by: Aditya Gidh <[email protected]>
@adigidh
add corresponding styles
8ff4603 
Signed-off-by: Aditya Gidh <[email protected]>
@adigidh
fix linting issues
65d4c80 
Signed-off-by: Aditya Gidh <[email protected]>
@adigidh
updates post git actions failures
5650a6e 
Signed-off-by: Aditya Gidh <[email protected]>
@adigidh
address UX code review updates
e618390 
Signed-off-by: Aditya Gidh <[email protected]>
@adigidh
updates for the knowledge review form
1f83349 
Signed-off-by: Aditya Gidh <[email protected]>
@adigidh
rebase and push with some comments on css file
6e98088 
Signed-off-by: Aditya Gidh <[email protected]>
@adigidh
minor edits
a0de003 
Signed-off-by: Aditya Gidh <[email protected]>
@adigidh
address code review comments
24e1dca 
Signed-off-by: Aditya Gidh <[email protected]>
@adigidh
adopt vars instead of absolute values
22b1e64 
Signed-off-by: Aditya Gidh <[email protected]>
@adigidh
adopt patternfly content component
ff6cbdb 
Signed-off-by: Aditya Gidh <[email protected]>
@adigidh
Copy link
Contributor Author

adigidh commented Feb 26, 2025

@adigidh can you please rebase your PR on main now ?

@vishnoianil : just rebased and verified the new changes from main are in place and in sync with this PR.

@vishnoianil vishnoianil merged commit 04b491b into instructlab:main Feb 26, 2025
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jeff-phillips-18 jeff-phillips-18 jeff-phillips-18 left review comments

@Misjohns Misjohns Misjohns approved these changes

@vishnoianil vishnoianil Awaiting requested review from vishnoianil

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Prettify the review submission step
4 participants
@adigidh @Misjohns @vishnoianil @jeff-phillips-18