Labels editing and validation #3426

YuliaKrimerman · 2024-11-04T15:13:00Z

Description

[

Screen.Recording.2024-11-06.at.1.31.09.PM.mov

](url)

When going to model/version Detail, editing the labels are behaving as inline labels and also now have a validation for a case a new label already exists ( side not the background of the error message is red in paternfly )

How Has This Been Tested?

on the UI

Test Impact

Working on adding test while getting feedback on the implementation

Request review criteria:

Self checklist (all need to be checked):

The developer has manually tested the changes and verified that the changes work
Testing instructions have been added in the PR body (for PRs involving changes that are not immediately obvious).
The developer has added tests or explained why testing cannot be added (unit or cypress tests for related changes)

If you have UI changes:

Included any necessary screenshots or gifs if it was a UI change.
Included tags to the UX team if it was a UI/UX change.

After the PR is posted & before it merges:

The developer has tested their solution on a cluster by using the image produced by the PR to main

YuliaKrimerman · 2024-11-04T15:14:08Z

@yih-wang Haley please take a look as well, and let me know what you think

mturley

One quick comment from looking at the video - I will give a more thorough review when I get a chance

mturley · 2024-11-04T21:22:44Z

frontend/src/components/EditableLabelsDescriptionListGroup.tsx

+                  editableProps={{
+                    'aria-label': 'Add label',
+                    defaultValue: '',
+                    'data-testid': 'add-label-input',
+                  }}


The way I understand the mockup, I don't think we want the "Add label" button to be an editable label itself. It can just be a button like before, but instead of its onClick opening a modal, it should add a new label called "New label" via setUnsavedLabels(). Then the user can click that label to edit it.

+1 to @mturley.
@YuliaKrimerman here's a PF demo of how should the Add label work: https://www.patternfly.org/components/label/#editable-label-group-with-add-button Hope it helps.

frontend/src/components/EditableLabelsDescriptionListGroup.tsx

YuliaKrimerman · 2024-11-05T14:20:43Z

@mturley @yih-wang I think I got it now. Updated the video.

yih-wang · 2024-11-06T03:28:21Z

Thanks for the updates @YuliaKrimerman! My only comment is when handling duplicate label errors, the newly created label should be marked in red to indicate the error. From the video, it looks like the existing label is flagged instead.

yih-wang · 2024-11-06T03:33:46Z

And one question to the loading time - from the video the newly added label takes a while to load after saving. Would this delay also occur in the production environment, or is it only in development environment? It’s a bit confusing to me before the label is loaded, so if this is expected in production, adding a loading state might help improve clarity.

codecov · 2024-11-06T19:15:27Z

Codecov Report

Attention: Patch coverage is 79.54545% with 18 lines in your changes missing coverage. Please review.

Project coverage is 85.40%. Comparing base (1636d39) to head (0009b62).
Report is 124 commits behind head on main.

Files with missing lines	Patch %	Lines
.../components/EditableLabelsDescriptionListGroup.tsx	80.23%	17 Missing ⚠️
...egistry/screens/ModelVersions/ModelDetailsView.tsx	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3426      +/-   ##
==========================================
+ Coverage   85.12%   85.40%   +0.27%     
==========================================
  Files        1338     1353      +15     
  Lines       30127    31129    +1002     
  Branches     8274     8692     +418     
==========================================
+ Hits        25647    26587     +940     
- Misses       4480     4542      +62

Files with missing lines	Coverage Δ
...d/src/components/DashboardDescriptionListGroup.tsx	`100.00% <100.00%> (ø)`
...ns/ModelVersionDetails/ModelVersionDetailsView.tsx	`95.00% <ø> (ø)`
...egistry/screens/ModelVersions/ModelDetailsView.tsx	`80.00% <0.00%> (ø)`
.../components/EditableLabelsDescriptionListGroup.tsx	`79.80% <80.23%> (+29.15%)`	⬆️

... and 265 files with indirect coverage changes

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1636d39...0009b62. Read the comment docs.

yih-wang · 2024-11-07T09:39:43Z

Thanks for the updates @YuliaKrimerman! All look good now!

One thing that wasn’t part of the original design proposal but stood out to me as potentially confusing from the video - currently, after saving an edit, the label list keeps its original expanded/collapsed state. This means if the list was collapsed before editing, and the user enters the edit mode then saves any changes, the newly added label isn’t visible because the list stays collapsed. To avoid this potential confusion, it might be clearer to have the label list automatically expand after any edit is saved, regardless of its original collapsed/expanded state.

Anyway it doesn't belong to the original scope of the issue, and I don't know how much work it needs, so not sure whether we should create a new story to improve this experience separately in a future sprint @mturley

manaswinidas

@YuliaKrimerman can you add testing instructions in the "How has this been tested?" section of PR description?

manaswinidas · 2024-11-08T09:55:41Z

frontend/src/__tests__/cypress/cypress/tests/mocked/modelRegistry/modelVersionDetails.cy.ts

+      modelVersionDetails.findAddLabelButton().click();
+
+      // Check label exists and is visible
+      cy.findByTestId('editable-label-New Label').should('exist');


can findLabel('New label') be used instead ofeditable-label-New label? I believe findLabel was added for this?

manaswinidas · 2024-11-08T09:56:20Z

frontend/src/__tests__/cypress/cypress/tests/mocked/modelRegistry/modelVersionDetails.cy.ts

+      cy.findByTestId('editable-label-New Label').click();
+
+      // Type the new label
+      cy.get('input[data-testid="edit-label-input-New Label"]').should('exist');


same applies here: modelVersionDetails.findLabelInput('New Label') instead of 'input[data-testid="edit-label-input-New Label"]' and the following 2-3 lines...

manaswinidas · 2024-11-08T09:58:48Z

frontend/src/__tests__/cypress/cypress/tests/mocked/modelRegistry/modelVersionDetails.cy.ts

+
+      cy.findByTestId('editable-label-Testing label').click();
+
+      cy.get('input[data-testid="edit-label-input-Testing label"]')


manaswinidas · 2024-11-08T09:59:16Z

frontend/src/__tests__/cypress/cypress/tests/mocked/modelRegistry/modelVersionDetails.cy.ts

+    it('should handle label validation', () => {
+      modelVersionDetails.findEditLabelsButton().click();
+
+      cy.findByTestId('editable-label-Testing label').click();


use findLabelInput('Testing label') instead

manaswinidas · 2024-11-08T10:01:35Z

frontend/src/components/EditableLabelsDescriptionListGroup.tsx

-        isSavingEdits={isSavingEdits}
-        contentWhenEditing={
+    <DashboardDescriptionListGroup
+      data-testid="editable-labels-group"


Is this test-id necessary? I don't see this test-id used anywhere.

manaswinidas · 2024-11-08T10:02:16Z

frontend/src/components/EditableLabelsDescriptionListGroup.tsx

          <LabelGroup
-            data-testid="label-group"
+            data-testid="editable-label-group"


I don't see this testid used anywhere.

manaswinidas · 2024-11-08T10:04:05Z

frontend/src/pages/modelRegistry/screens/ModelVersions/ModelDetailsView.tsx

@@ -62,6 +64,7 @@ const ModelDetailsView: React.FC<ModelDetailsViewProps> = ({
                )
                .then(refresh)
            }
+            data-testid="model-labels"


I don't see this testid anywhere

mturley

Hey @YuliaKrimerman, progress is looking great here but we need a bit of refactoring.

mturley · 2024-11-08T17:51:20Z

frontend/src/components/EditableLabelsDescriptionListGroup.tsx

+    newText: string,
+    currentLabel?: string,
+  ) => {
+    const error = validateLabel(newText, currentLabel);


If I cause an error (e.g. add a duplicate label) and don't fix it, then add another label, the error disappears and submission is allowed even though there is still a problem. This is because we store errors in state and we can only have one at a time.

Errors don't need to be state here, they can be derived from the existing unsavedLabels state on each render. I think instead of having the labelErrors come from React.useState, we can just have a const labelErrors object that we compute as part of the rendering logic of the component. That way, simply the call to setUnsavedLabels that puts us in an error state will cause errors to appear. That does mean a bit of refactoring, since you won't be validating a specific label anymore but validating the list of labels. This also means we may have multiple errors to display - if you have two sets of duplicate labels, you'll want to call them both out.

@yih-wang Haley, we would need your input in this case on how to handle double duplicates, should we show them both like this?(Ignore the double red color of the labels- only the latests will be red and not all of them) Should we even allow multiple duplicates at edit time? Let me know what you think : this is how it looks with what Mike suggested ( if we want to take this route please let me know which text to use)

Hi @YuliaKrimerman, I've reviewed this pattern with Anges, who is finalizing the ux validation guidelines. Her suggestion is to allow multiple duplicates at the edit time.

@kaedward, I'd like to hear your input on the validation message here - Currently, we show the error message below when there's only one label duplicated
‘mylabel’ already exists. Use a unique name that does not match any existing label or property key.

But how should the error message look like when there're multiple duplicates? Should it be a general message like
Labels duplicated. Use a unique name that does not match any existing label or property key.
or a specific message which list all the duplicated labels' names like
‘mylabel’ and 'new' already exist. Use a unique name that does not match any existing label or property key.

@yih-wang How do these sound? Do we need to specify that the label name must not match a property?
The label name should be bolded, not in quotation marks.

ONE ERROR:
label-name appears more than once. Ensure that each label is unique.

MORE THAN 1 ERROR:
label-name and label-name2 appear more than once. Ensure that each label is unique.

Do we need to specify that the label name must not match a property?

@kaedward I think it would be helpful to specify that the label shouldn’t match any existing labels or property keys. This would prevent confusion when users don’t see duplicated labels but encounter issues due to a property key duplicating the label.

The pattern you suggested looks good to me. But I'm wondering whether we should say 'label-name already exists' rather than 'label-name appears more than once' because it's already used elsewhere for duplication validation.

ONE ERROR:
label-name already exists. Ensure that each label is unique and does not match any existing property key.

MORE THAN 1 ERROR:
label-name and label-name2 already exist. Ensure that each label is unique and does not match any existing property key.

@yih-wang Here ya go!

mturley · 2024-11-08T18:03:19Z

frontend/src/components/EditableLabelsDescriptionListGroup.tsx

+    if (error) {
+      setLabelErrors({ [newText]: error });
+      setUnsavedLabels((prev) => {
+        const filtered = prev.filter((label) => label !== currentLabel);
+        return [...filtered, newText];
+      });
+    } else if (newText) {
+      setUnsavedLabels((prev) => {
+        if (currentLabel) {
+          return [...prev, newText];
+        }
+        const filtered = prev.filter((label) => label !== currentLabel);
+        return [...filtered, newText];
+      });
+      setLabelErrors({});
+    }
+  };


I'm not sure why the setUnsavedLabels behavior is different if there's an error vs if there isn't. This may resolve itself if you move the validation from here to render time, but I'm pretty sure handleEditComplete should update the unsaved labels state the same way no matter whether there is an error to show. I think this first call in your if (error) case is correct:

setUnsavedLabels((prev) => { const filtered = prev.filter((label) => label !== currentLabel); return [...filtered, newText]; });

Your other case is making it so any time edit focus is lost, if the user didn't enter duplicate label text, the old label is not filtered out on update and we end up with another duplicate:

Screen.Recording.2024-11-08.at.12.56.50.PM.mov

yih-wang · 2024-11-19T04:32:04Z

@YuliaKrimerman When I reviewed the validation pattern with Anges, she pointed out another inconsistency with the upcoming validation guidelines - the ✓ should always remain enabled even when there're duplication errors. This provides better accessibility and aligns with the new form validation pattern.

Apologies for the adjustment from the original design, I think it's better to address this comment and change the ✓ behavior to be always enabled.

To be specific, if user tries to save (clicking ✓) when there're errors, "the focus should shift from the submit button to the contents in the inline alert (plain alert in our case). This enables screen reader users to hear the error message, and then easily navigate to the fields in the form where the errors exist." (quoted from the validation guidelines)

YuliaKrimerman · 2024-11-20T18:11:45Z

Hi @yih-wang @kaedward @mturley. Please take a look at the video recording. It has all the requested changes so far : Handling double duplications, proper error display based on amount of duplications and enabling the save button even when error exist. Also added some top padding above the red alert as Mike suggested (see screenshot, not video). Mike if you have a chance you can also do a re-review while I will work on fixing the last test, and brace for any incoming comments if needed.
https://github.com/user-attachments/assets/ce4bfed8-d39b-476c-a408-da627a19d74e

mturley · 2024-11-20T18:27:03Z

@YuliaKrimerman I'll do a full review asap, but just looking at that screenshot it looks like you lost the red color on the errored labels?

YuliaKrimerman · 2024-11-20T18:29:30Z

@mturley Haha no, it's just I screenshot with only the original showing, and not the duplicate. In the video the red is there :)

mturley · 2024-11-20T18:37:18Z

Ohh, I misread sorry!

yih-wang · 2024-11-21T10:17:43Z

Thanks @YuliaKrimerman, the latest updates look good!

The only question I have is do we decide whether or not address my earlier comment with this pr? Just make sure it's not overlooked.

One thing that wasn’t part of the original design proposal but stood out to me as potentially confusing from the video - currently, after saving an edit, the label list keeps its original expanded/collapsed state. This means if the list was collapsed before editing, and the user enters the edit mode then saves any changes, the newly added label isn’t visible because the list stays collapsed. To avoid this potential confusion, it might be clearer to have the label list automatically expand after any edit is saved, regardless of its original collapsed/expanded state.

Anyway it doesn't belong to the original scope of the issue, and I don't know how much work it needs, so not sure whether we should create a new story to improve this experience separately in a future sprint

YuliaKrimerman · 2024-11-21T14:17:51Z

@yih-wang Yeah I didn't address it as part of this issue. I think opening a separate story for this might be a good idea.

mturley

@YuliaKrimerman I'm so sorry for the delay, don't kill me, I have comments.....

mturley · 2024-11-21T22:21:49Z

frontend/src/components/EditableLabelsDescriptionListGroup.tsx

+    const duplicateLabels: string[] = [];
+    duplicatesMap.forEach((count, label) => {
+      if (count > 1) {
+        duplicateLabels.push(label);


@YuliaKrimerman I hate to drag this out a bit longer but can we add a test or two that covers the duplicate label validation to make codecov happier?

mturley · 2024-11-21T22:29:21Z

frontend/src/components/EditableLabelsDescriptionListGroup.tsx

+    }
+
+    labelsList.forEach((label) => {
+      if (!labelsList.includes(label) && allExistingKeys.includes(label)) {


This logic is wrong. !labelsList.includes(label) will always be false here because we're looping over labelsList, the label will always be in labelsList. So this validation case is never hit, and as a user I am allowed to create a label that already exists as a key in the properties table below, which replaces it and loses that property value.

I think instead, we need to either consider allExistingKeys when we are setting up the duplicatesMap or we need to have this logic be somehow if (weDontAlreadyHaveAnErrorForThisLabel && allExistingKeys.includes(label)). In that latter case, the message could be specific to the fact that the label matches an existing property key, like **${label}** already exists as a property key. Ensure that each label is unique and does not match any existing property key.

mturley · 2024-11-21T22:30:46Z

frontend/src/components/EditableLabelsDescriptionListGroup.tsx

+    setUnsavedLabels((prev) => [...prev, newLabel]);
+  };
+
+  const labelErrors = validateLabels(unsavedLabels);


validateLabels doesn't really need to take unsavedLabels as an argument at all, it can just directly use unsavedLabels in its body instead of its labelsList.

mturley · 2024-11-21T22:41:35Z

frontend/src/components/EditableLabelsDescriptionListGroup.tsx

+              style={{
+                border: '2px solid #d2d2d2',
+                color: '#0066CC',
+                backgroundColor: 'transparent',
+              }}


These inline styles override the border/color at all times so there is no longer a hover state. We should avoid inline styles where possible.

I don't think we need our own Button here. Can we not use the addLabelControl prop of LabelGroup like we had before? That automatically adds a button with the correct hover styles while editing like in this PF example. https://www.patternfly.org/components/label#editable-label-group
I locally reverted that part of this change and put back that addLabelControl but just had it call your new addNewLabel function, and it appears to work fine.

mturley · 2024-11-21T22:47:36Z

frontend/src/components/EditableLabelsDescriptionListGroup.tsx

+              aria-live="polite"
+              isPlain
+              tabIndex={-1}
+              style={{ marginTop: '16px' }}


Instead of inline styles here, you can import spacing from '@patternfly/react-styles/css/utilities/Spacing/spacing'; and then use className={spacing.mtMd} which will set a margin-top to the PF medium spacer, which is 16px.

YuliaKrimerman · 2024-11-26T18:46:56Z

@mturley Ready for re-review. Added another error type for our edge case

mturley · 2024-11-26T19:15:45Z

frontend/src/components/EditableLabelsDescriptionListGroup.tsx

+            numLabels={10}
+            expandedText="Show Less"
+            collapsedText="Show More"


Ooooone last thing - we should set numLabels back to unsavedLabels.length and remove the expandedText and collapsedText props. When editing labels they should always be expanded, the show more/less behavior is only when they are not being edited which is handled by the other LabelGroup rendered in non-edit mode. Otherwise, if they are collapsed and you click "Add label" the new label doesn't appear until you expand.

Signed-off-by: Mike Turley <[email protected]>

mturley

One more thing --- just kidding, approved

openshift-ci · 2024-11-26T20:38:26Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: gitdallas, mturley

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:

~~frontend/OWNERS~~ [mturley]

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

YuliaKrimerman added 2 commits October 30, 2024 16:34

saving

11aa082

saving

62c8032

openshift-ci bot added the do-not-merge/work-in-progress This PR is in WIP state label Nov 4, 2024

openshift-ci bot requested review from dpanshug and ppadti November 4, 2024 15:13

lint errors fix

14ff57d

mturley reviewed Nov 4, 2024

View reviewed changes

ppadti reviewed Nov 5, 2024

View reviewed changes

frontend/src/components/EditableLabelsDescriptionListGroup.tsx Outdated Show resolved Hide resolved

addressed comments

3dfe1d3

test

f6a8462

YuliaKrimerman added 2 commits November 6, 2024 09:38

fixed last label red

d10592a

added tests

be6cb6d

YuliaKrimerman changed the title ~~WIP - Labels editing and validation~~ Labels editing and validation Nov 7, 2024

openshift-ci bot removed the do-not-merge/work-in-progress This PR is in WIP state label Nov 7, 2024

manaswinidas requested changes Nov 8, 2024

View reviewed changes

openshift-ci bot assigned manaswinidas Nov 8, 2024

manaswinidas requested changes Nov 8, 2024

View reviewed changes

Manaswini's comments addressed

eed547a

mturley requested changes Nov 8, 2024

View reviewed changes

openshift-ci bot assigned mturley Nov 8, 2024

YuliaKrimerman added 2 commits November 19, 2024 15:21

saving

ea6f439

addressed new requirments

23c1ead

fix test

1b1001f

gitdallas approved these changes Nov 21, 2024

View reviewed changes

openshift-ci bot assigned gitdallas Nov 21, 2024

openshift-ci bot added the lgtm label Nov 21, 2024

mturley requested changes Nov 21, 2024

View reviewed changes

openshift-ci bot removed the lgtm label Nov 21, 2024

mturley reviewed Nov 21, 2024

View reviewed changes

latest version

efbb326

final

7e7dd5e

mturley requested changes Nov 26, 2024

View reviewed changes

YuliaKrimerman and others added 2 commits November 26, 2024 14:49

final1

92b54b9

Force LabelGroup to expand after saving edits

0009b62

Signed-off-by: Mike Turley <[email protected]>

mturley approved these changes Nov 26, 2024

View reviewed changes

openshift-ci bot added the lgtm label Nov 26, 2024

openshift-ci bot added the approved label Nov 26, 2024

openshift-merge-bot bot merged commit 6b1b8e5 into opendatahub-io:main Nov 26, 2024
6 checks passed


		cy.findByTestId('editable-label-Testing label').click();

		cy.get('input[data-testid="edit-label-input-Testing label"]')

Labels editing and validation #3426

Labels editing and validation #3426

Conversation

YuliaKrimerman commented Nov 4, 2024 • edited Loading

Description

How Has This Been Tested?

Test Impact

Request review criteria:

YuliaKrimerman commented Nov 4, 2024

mturley left a comment

Choose a reason for hiding this comment

mturley Nov 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

YuliaKrimerman commented Nov 5, 2024

yih-wang commented Nov 6, 2024

yih-wang commented Nov 6, 2024

codecov bot commented Nov 6, 2024 • edited Loading

Codecov Report

yih-wang commented Nov 7, 2024

manaswinidas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mturley left a comment

Choose a reason for hiding this comment

mturley Nov 8, 2024 • edited Loading

Choose a reason for hiding this comment

YuliaKrimerman Nov 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yih-wang Nov 20, 2024 • edited Loading

Choose a reason for hiding this comment

kaedward Nov 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yih-wang commented Nov 19, 2024 • edited Loading

YuliaKrimerman commented Nov 20, 2024

mturley commented Nov 20, 2024

YuliaKrimerman commented Nov 20, 2024

mturley commented Nov 20, 2024

yih-wang commented Nov 21, 2024

YuliaKrimerman commented Nov 21, 2024

mturley left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mturley Nov 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

YuliaKrimerman commented Nov 26, 2024

Choose a reason for hiding this comment

mturley left a comment

Choose a reason for hiding this comment

openshift-ci bot commented Nov 26, 2024

YuliaKrimerman commented Nov 4, 2024 •

edited

Loading

mturley Nov 4, 2024 •

edited

Loading

codecov bot commented Nov 6, 2024 •

edited

Loading

mturley Nov 8, 2024 •

edited

Loading

YuliaKrimerman Nov 18, 2024 •

edited

Loading

yih-wang Nov 20, 2024 •

edited

Loading

kaedward Nov 20, 2024 •

edited

Loading

yih-wang commented Nov 19, 2024 •

edited

Loading

mturley Nov 21, 2024 •

edited

Loading