523: UI updates for the submission steps on "Skill" & "Knowledge" Review forms#581
523: UI updates for the submission steps on "Skill" & "Knowledge" Review forms#581vishnoianil merged 15 commits intoinstructlab:mainfrom
Conversation
adigidh
force-pushed
the
523-review-step
branch
from
20920d0 to
8891d2a
Compare
vishnoianil
requested review from
Misjohns,
jeff-phillips-18 and
vishnoianil
|
@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. The accordion header should match content frame color unless the user is hovering over the header.
Expanded but no hover Users cursor on accordion header |
Misjohns
left a comment
Misjohns
left a comment
There was a problem hiding this comment.
Left my comments in previous message.
adigidh
force-pushed
the
523-review-step
branch
from
9641a09 to
d2e745d
Compare
|
Thank you for taking a look. Made the following updates @Misjohns :
hover.mov |
adigidh
changed the title
adigidh
force-pushed
the
523-review-step
branch
2 times, most recently
from
faba118 to
24705d2
Compare
|
@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. |
| </p> | ||
| <article> | ||
| <div className="info-wrapper"> | ||
| <p className="submission-titles">Author Information</p> |
There was a problem hiding this comment.
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.
| </p> | ||
| <article> | ||
| {/* File Path Information */} | ||
| <h5 className="category-titles">File Path Information</h5> |
There was a problem hiding this comment.
Let's use Directory Path, similar to skill form.
| <p>{knowledgeFormData.knowledgeDocumentRepositoryUrl}</p> | ||
| <h5 className="category-titles">Commit</h5> | ||
| <p>{knowledgeFormData.knowledgeDocumentCommit}</p> | ||
| <h5 className="category-titles">Commit</h5> |
| <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> |
| <> | ||
| <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> |
There was a problem hiding this comment.
This is skill review form, so we need to remove all the reference for "Knowledge" through out this form.
| {/* Seed Examples */} | ||
| <article> | ||
| <div className="info-wrapper"> | ||
| <p>Seed data</p> |
There was a problem hiding this comment.
Let's use "Seed Example" here.
adigidh
force-pushed
the
523-review-step
branch
from
ba00d62 to
09a4e1e
Compare
|
Thank you for taking a look, and the detailed review.
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. |
| {/* Author Information */} | ||
| <article> | ||
| <div className="info-wrapper"> | ||
| <p className="submission-titles">Contributor Information</p> |
There was a problem hiding this comment.
We should use <Content component={ContentVariants.p} instead of direct <p> elements
| </div> | ||
|
|
||
| <div className="contributors-wrapper"> | ||
| <h5 className="category-titles">Contributors</h5> |
There was a problem hiding this comment.
We should use <Content component={ContentVariants.h5} instead of direct <h5> components.
| import { Content, ContentVariants } from '@patternfly/react-core'; | ||
| import React from 'react'; | ||
| import './submission.css'; | ||
| import { Accordion, AccordionContent, AccordionItem, AccordionToggle } from '@patternfly/react-core'; |
There was a problem hiding this comment.
Add these to the imports from @patternfly/react-core above
| {/* Author Information */} | ||
| <article> | ||
| <div className="info-wrapper"> | ||
| <p className="submission-titles">Contributor Information</p> |
There was a problem hiding this comment.
We should use the Content components here as well
| .review-submission-container { | ||
| .contributors-wrapper { | ||
| line-height: 2; | ||
| padding-top: 15px; |
There was a problem hiding this comment.
| padding-top: 15px; | |
| padding-top: var(--pf-t--global--spacer--md); |
| } | ||
|
|
||
| .accordion-wrapper { | ||
| padding-bottom: 15px; |
There was a problem hiding this comment.
| padding-bottom: 15px; | |
| padding-bottom: var(--pf-t--global--spacer--md); |
| } | ||
|
|
||
| .category-titles { | ||
| font-weight: 600; |
There was a problem hiding this comment.
| font-weight: 600; | |
| font-weight: var(pf-t--global--font--weight--heading); |
| } | ||
|
|
||
| .accordion-content { | ||
| padding: 15px; |
There was a problem hiding this comment.
| padding: 15px; | |
| padding: var(--pf-t--global--spacer--md); |
| .seed-category-titles { | ||
| font-weight: 600; | ||
| font-size: var(--pf-t--global--font--size--xs) !important; | ||
| padding-bottom: 5px; |
There was a problem hiding this comment.
| padding-bottom: 5px; | |
| padding-bottom: var(--pf-t--global--spacer--sm); |
| } | ||
|
|
||
| .seed-category-titles { | ||
| font-weight: 600; |
There was a problem hiding this comment.
| font-weight: 600; | |
| font-weight: var(pf-t--global--font--weight--heading); |
adigidh
force-pushed
the
523-review-step
branch
from
741eaad to
1c62aa6
Compare
|
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. |
|
@adigidh can you please rebase your PR on main now ? |
Signed-off-by: Aditya Gidh <aagidh@us.ibm.com>
Signed-off-by: Aditya Gidh <aagidh@us.ibm.com>
Signed-off-by: Aditya Gidh <aagidh@us.ibm.com>
Signed-off-by: Aditya Gidh <aagidh@us.ibm.com>
Signed-off-by: Aditya Gidh <aagidh@us.ibm.com>
Signed-off-by: Aditya Gidh <aagidh@us.ibm.com>
Signed-off-by: Aditya Gidh <aagidh@us.ibm.com>
Signed-off-by: Aditya Gidh <aagidh@us.ibm.com>
Signed-off-by: Aditya Gidh <aagidh@us.ibm.com>
Signed-off-by: Aditya Gidh <aagidh@us.ibm.com>
Signed-off-by: Aditya Gidh <aagidh@us.ibm.com>
Signed-off-by: Aditya Gidh <aagidh@us.ibm.com>
Signed-off-by: Aditya Gidh <aagidh@us.ibm.com>
Signed-off-by: Aditya Gidh <aagidh@us.ibm.com>
Signed-off-by: Aditya Gidh <aagidh@us.ibm.com>
adigidh
force-pushed
the
523-review-step
branch
from
f1c2970 to
ff6cbdb
Compare
@vishnoianil : just rebased and verified the new changes from main are in place and in sync with this PR. |
4 participants
fixes #523
Summary of changes on both skill and knowledge review sections:
Screen recording ZIP:
instructlab.mov.zip