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

Apply MUI theming to Register Model Form #451

Merged
merged 1 commit into from
Oct 9, 2024
Merged

Apply MUI theming to Register Model Form #451

merged 1 commit into from
Oct 9, 2024

Conversation

jenny-s51
Copy link
Contributor

Description

Applies theming and styles to MR UI for Registered Model Form.

Before:
Screenshot 2024-10-04 at 10 10 40 AM

After:
Screenshot 2024-10-04 at 10 09 38 AM

How Has This Been Tested?

Visual updates only - tested manually in UI. Navigate to Model Registry list view and click "Register Model" button in toolbar.

Merge criteria:

  • All the commits have been signed-off (To pass the DCO check)
  • The commits have meaningful messages; the author will squash them after approval or in case of manual merges will ask to merge with squash.
  • Testing instructions have been added in the PR body (for PRs involving changes that are not immediately obvious).
  • The developer has manually tested the changes and verified that the changes work.
  • Code changes follow the kubeflow contribution guidelines.

If you have UI changes

  • The developer has added tests or explained why testing cannot be added.
  • Included any necessary screenshots or gifs if it was a UI change.
  • Verify that UI/UX changes conform the UX guidelines for Kubeflow.

@jenny-s51 jenny-s51 marked this pull request as draft October 4, 2024 14:43
@jenny-s51 jenny-s51 force-pushed the registerModelForm branch 2 times, most recently from 399fa3e to 7c69950 Compare October 7, 2024 18:58
@jenny-s51 jenny-s51 marked this pull request as ready for review October 7, 2024 18:59
@Griffin-Sullivan
Copy link
Contributor

It looks great! Found one bug though with the description text boxes:

image

Dragging the box to resize it leaves the input width the same, but will move the text and label with it.

jenny-s51
jenny-s51 commented Oct 7, 2024
View reviewed changes
Copy link
Contributor Author

@jenny-s51 jenny-s51 left a comment

Choose a reason for hiding this comment

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

Nice catch, thank you @Griffin-Sullivan ! Resize is disabled for text areas in MUI - updated the styles to reflect this.

@jenny-s51 jenny-s51 force-pushed the registerModelForm branch 3 times, most recently from 69f59d5 to 1f87981 Compare October 8, 2024 15:33
Griffin-Sullivan
Griffin-Sullivan approved these changes Oct 8, 2024
View reviewed changes
Copy link
Contributor

@Griffin-Sullivan Griffin-Sullivan left a comment

Choose a reason for hiding this comment

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

LGTM. One enhancement code wise and a question

...rontend/src/app/pages/modelRegistry/screens/RegisterModel/RegistrationCommonFormSections.tsx Outdated
Comment on lines 293 to 300
<div className="form-fieldset-wrapper">
{pathInput}
<fieldset aria-hidden="true" className="form-fieldset">
<legend className="form-fieldset-legend">
<span>Path</span>
</legend>
</fieldset>
</div>
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this could be cleaner if we add a simple component for this and reuse it for each FormGroup.

Copy link
Contributor

Choose a reason for hiding this comment

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

Even just a small component function at the top of the page if we only need it for this file. If it can be used in other places then we can make it a common component.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point, thanks @Griffin-Sullivan! Updated this fieldset wrapper to be a reusable component.

clients/ui/frontend/src/app/pages/modelRegistry/screens/RegisterModel/RegisterModel.tsx Outdated
Comment on lines 124 to 131
<div className="form-fieldset-wrapper">
{modelDescriptionInput}
<fieldset aria-hidden="true" className="form-fieldset">
<legend className="form-fieldset-legend">
<span>Model Description</span>
</legend>
</fieldset>
</div>
Copy link
Contributor

Choose a reason for hiding this comment

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

Why does this structure need to be added? Is this how we get the label to get inserted into the TextInput? Not against anything here just looking to learn.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great question! Yes, this applies the nested label within the input border, which couldn't be achieved with pure CSS using PF's default component structure. MUI uses fieldset to achieve this styling which we applied here to get the same look and feel on the text fields.

@jenny-s51 jenny-s51 force-pushed the registerModelForm branch 3 times, most recently from f033a9d to 74bb5a2 Compare October 8, 2024 20:00
@jenny-s51 jenny-s51 mentioned this pull request Oct 8, 2024
Closed
@jenny-s51 jenny-s51 force-pushed the registerModelForm branch 2 times, most recently from d1d2957 to f6f7428 Compare October 9, 2024 13:42
@jenny-s51
Apply MUI theming to Register Model form
d269ac3 
Signed-off-by: Jenny <[email protected]>

fix tests

Signed-off-by: Jenny <[email protected]>

add fieldset to custom components

fix spacing issue with helper text

Signed-off-by: Jenny <[email protected]>

remove comment

Signed-off-by: Jenny <[email protected]>

fixes for text area

Signed-off-by: Jenny <[email protected]>

format

Signed-off-by: Jenny <[email protected]>

add model description fix

Signed-off-by: Jenny <[email protected]>

create reusable fieldset component

Signed-off-by: Jenny <[email protected]>

move fieldset to components

Signed-off-by: Jenny <[email protected]>

add prop for className

Signed-off-by: Jenny <[email protected]>

lint

Signed-off-by: Jenny <[email protected]>

fix import warnings

fix TS error with FormFieldset.tsx

Signed-off-by: Jenny <[email protected]>

fix import order errors
lucferbux
lucferbux approved these changes Oct 9, 2024
View reviewed changes
Copy link
Contributor

@lucferbux lucferbux left a comment

Choose a reason for hiding this comment

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

/lgtm

@google-oss-prow Google OSS Prow
Copy link

@lucferbux: changing LGTM is restricted to collaborators

In response to this:

/lgtm

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@google-oss-prow Google OSS Prow
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ederign, Griffin-Sullivan, lucferbux

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@ederign
Copy link
Member

ederign commented Oct 9, 2024

/lgtm

@google-oss-prow google-oss-prow bot added the lgtm label Oct 9, 2024
@google-oss-prow google-oss-prow bot merged commit 661cb25 into kubeflow:main Oct 9, 2024
16 checks passed
@jenny-s51 jenny-s51 mentioned this pull request Oct 10, 2024
Closed
7 tasks
@tarilabs tarilabs mentioned this pull request Oct 15, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Griffin-Sullivan Griffin-Sullivan Griffin-Sullivan approved these changes

@lucferbux lucferbux lucferbux approved these changes

@ederign ederign ederign approved these changes

@alexcreasy alexcreasy Awaiting requested review from alexcreasy

@andreyvelich andreyvelich Awaiting requested review from andreyvelich

Assignees

@ederign ederign

Labels
approved Area/UI lgtm size/XL
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@jenny-s51 @Griffin-Sullivan @ederign @lucferbux