Add labels to CreatePipelineRun modal by ncskier · Pull Request #699 · tektoncd/dashboard

ncskier · 2019-11-05T22:43:36Z

Changes

Fixes #323
This PR turns the Annotations component into a reusable KeyValueList
component. This KeyValueList component is used to manage annotations in
the SecretsModal, and it is used in the CreatePipelineRun modal to
manage labels.

Here's an image of the labels in the CreatePipelineRun modal (feedback is welcome 🙂):

/cc @AlanGreene
/cc @carlos-logro

Submitter Checklist

These are the criteria that every PR should meet, please check them off as you
review them:

Includes tests (if functionality changed/added)
Includes docs (if user facing)
Commit messages follow commit message best practices

See the contribution guide
for more details.

carlos-logro

Great PR Brandon! Most of my feedback are suggestions and minor changes you missed in the new test file.

carlos-logro · 2019-11-06T21:27:50Z

-            .toString(36)
-            .substring(2, 11)
+          valuePlaceholder: 'https://github.com',
+          id: 'annotation0'


as per Alan Green's comment on using index or array length for key value:

key should be a value derived from the data being rendered as react uses it to determine which elements need to be updated during the render cycle in response to data change, using the array index is not a good idea as it can lead to unexpected behaviour.

Ok... it was this before

Math.random() .toString(36) .substr(2, 9);

so I'll update the id to this in the SecretsModal and CreatePipelineRun modal.

carlos-logro · 2019-11-06T21:49:06Z

      annotations: [
        {
-          label: `tekton.dev/git-0`,
+          key: 'tekton.dev/git-0',


what's the difference between key & keyPlaceholder?

The key, is the actual text that is in the "key" TextInput (the value prop), whereas the keyPlaceholder is the placeholder text in the "key" TextInput (the placeholder prop).

Do you think that these names are confusing/do you have an idea for an alternative name?

AlanGreene · 2019-11-07T13:38:54Z

I'd like to get some design input on the controls for adding/removing items from the list, in particular for removal... IMO having the remove button at the top of the list remove an item from the bottom of the list seems odd. Since this is a pattern we're starting to reuse in multiple places in the UI we should make sure we're not causing confusion.

ncskier · 2019-11-07T18:54:12Z

Thanks for the thorough feedback @carlos-logro & @AlanGreene 🙂 I have updated the code to addressed all of your comments.

I'd like to get some design input on the controls for adding/removing items from the list, in particular for removal... IMO having the remove button at the top of the list remove an item from the bottom of the list seems odd. Since this is a pattern we're starting to reuse in multiple places in the UI we should make sure we're not causing confusion.

@AlanGreene you make a good point about the remove behavior being a little unintuitive. Fortunately, since the KeyValueList is now a reusable component, it will be relatively easy to modify 😁 Since we already have this component in the Dashboard, would it be alright if we handle this redesign under a separate issue (separate from this PR)? I think that it could get messy redesigning the component under this PR, because this PR is primarily concerned with adding labels to the Create PipelineRun modal.

AlanGreene · 2019-11-11T15:44:00Z

Hey @ncskier, yes let's create an issue to make sure that's tracked and try to get to it soon. I'll take another looks at your changes this evening.

AlanGreene · 2019-11-12T11:50:29Z

+              : 'removeIcon'
+          }
+          onClick={onRemove}
+          aria-label={`Remove Button. Removes ${legendText} entry.`}


same as above, would this be useful for all users?

Sorry, I'm not sure what you mean by this. What would I do differently if it was useful or wasn't useful for all users?

aria-label will only be read by screen readers or similar assistive technologies. If the text in this label is useful / intended for all users it should be a tooltip, an actual label, or some other visible element on the page.

Ah, gotcha, thanks for clarifying. I personally don't think that the text will help users without screen readers. I think that the icons on the buttons are pretty self-explanatory.

AlanGreene · 2019-11-12T11:55:41Z

 }
+
+// Generates a unique id
+export function generateId() {


If we need to guarantee uniqueness we should either use a UUID or add additional context to reduce likelihood of a conflict with other elements on the page that may be difficult to debug. Do we already have another value we could use instead? e.g. are the keys required to be unique already?

Yes, it used the index in the list before, but I received this comment, so I changed it to the generateId() function.

I'm a little confused about this, but I added a prefix for each id based on the item in the KeyValueList; for example:

id: generateId(`annotation${annotations.length}-`)

Do you think this is alright?

AlanGreene · 2019-11-12T12:00:59Z

              />
            </FormGroup>
          )}
+          <FormGroup legendText="">


should the legend text be applied to the FormGroup instead or can we remove the group here?

The current style has the legend text inline with the Add & Remove buttons. We can't do this with a normal FormGroup legendText. So, instead, I'm using the styles that were in the SecretModal which make it look like a FormGroup's legendText.

ncskier · 2019-11-22T14:01:04Z

Thanks for the review @AlanGreene! I think that I've addressed all of your comments. Can you please take another look when you get the chance?

AlanGreene · 2019-11-25T14:02:13Z

+            { legendText }
+          )}
+        />
+        <Add


Sorry I didn't pick up on this in previous reviews, I didn't realise these were just icons and not buttons.
We should use Button components here with the icon prop set, this will ensure keyboard accessibility etc. works as expected. If you want to hide the button border and background you can set kind="ghost". See LabelFilter, LogoutButton, or Rerun for examples.

AlanGreene · 2019-11-25T14:07:06Z

+                defaultMessage: 'Labels'
+              })}
+              invalidText={intl.formatMessage(
+                {


We can't translate fragments of a sentence in isolation (e.g. 'kubernetes labels syntax'), it won't work for many languages.
Instead what we want is to add placeholders for the link markup so the sentence can be translated as a whole, with the correct context for each part.

Something like this:
'Labels must follow the {0}kubernetes labels syntax{1}.'

@AlanGreene I don't think it lets me split up JSX tags like that:

(is this what you were suggesting?)

I could maybe do something like this to use an icon instead of text as the link, so we won't have to translate it?

ok looks like react-intl decided this was something people don't need, but there's still a way to get what we want. React's scarily named dangerouslySetInnerHTML to the rescue 😆 In this case it's safe as we fully control the content, there's no user data involved, we can consider the HTML content sanitised.

invalidText={ <span dangerouslySetInnerHTML /* eslint-disable-line react/no-danger */={{ __html: intl.formatMessage( { id: 'dashboard.createPipelineRun.labels.invalidText', defaultMessage: 'Labels must follow the {0}kubernetes labels syntax{1}.' }, [ `<a href="https://kubernetes.io/docs/concepts/overview/working-with-objects/labels/#syntax-and-character-set" target="_blank" rel="noopener noreferrer" >`, '</a>' ] ) }} /> }

Awesome, thanks Alan! I just made the change.

AlanGreene · 2019-11-25T14:09:46Z

    "dashboard.errorBoundary.pageError": "Error loading page",
    "dashboard.header.logOut": "Log out",
+    "dashboard.keyValueList.addButton.ariaLabel": "Add Button. Adds {legendText} entry.",
+    "dashboard.keyValueList.deleteButton.ariaLabel": "Delete Button#{index}. Deletes entire entry in current text field.",


I'm still struggling a bit with these strings.

'Delete Button#0', will this make sense to a user?
'current text field', how do I know which is the 'current' text field?

You're right, those strings don't really make sense. Since there's already text on the screen for each of these elements, I'm going to remove the aria labels. Is that alright?

Fixes tektoncd#323 This PR turns the Annotations component into a reusable KeyValueList component. This KeyValueList component is used to manage annotations in the SecretsModal, and it is used in the CreatePipelineRun modal to manage labels.

AlanGreene

/lgtm

tekton-robot · 2019-12-06T19:00:01Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: AlanGreene

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

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [AlanGreene]

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

tekton-robot requested review from AlanGreene and carlos-logro November 5, 2019 22:43

tekton-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Nov 5, 2019

ncskier force-pushed the issue323 branch from bb30c88 to e00b9a0 Compare November 5, 2019 23:10

ncskier marked this pull request as ready for review November 5, 2019 23:27

tekton-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 5, 2019

ncskier force-pushed the issue323 branch from e00b9a0 to 816a85c Compare November 6, 2019 18:15

carlos-logro suggested changes Nov 6, 2019

View reviewed changes

ncskier force-pushed the issue323 branch from 816a85c to be2d61c Compare November 7, 2019 18:40

ncskier mentioned this pull request Nov 11, 2019

KeyValueList component unintuitive design #716

Closed

AlanGreene requested changes Nov 12, 2019

View reviewed changes

ncskier force-pushed the issue323 branch 7 times, most recently from c4a8681 to eef6064 Compare November 21, 2019 20:45

ncskier requested a review from AlanGreene November 21, 2019 20:46

AlanGreene requested changes Nov 25, 2019

View reviewed changes

ncskier force-pushed the issue323 branch 5 times, most recently from c0f29c4 to 1dc033c Compare December 5, 2019 22:15

ncskier requested a review from AlanGreene December 5, 2019 22:16

ncskier force-pushed the issue323 branch from 1dc033c to 41780d8 Compare December 6, 2019 14:31

Add labels to CreatePipelineRun modal

ebf3341

Fixes tektoncd#323 This PR turns the Annotations component into a reusable KeyValueList component. This KeyValueList component is used to manage annotations in the SecretsModal, and it is used in the CreatePipelineRun modal to manage labels.

ncskier force-pushed the issue323 branch from 41780d8 to ebf3341 Compare December 6, 2019 18:42

AlanGreene approved these changes Dec 6, 2019

View reviewed changes

tekton-robot assigned AlanGreene Dec 6, 2019

tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Dec 6, 2019

tekton-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 6, 2019

tekton-robot merged commit b17e603 into tektoncd:master Dec 6, 2019

Uh oh!

Conversation

ncskier commented Nov 5, 2019

Changes

Submitter Checklist

Uh oh!

carlos-logro left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlanGreene commented Nov 7, 2019

Uh oh!

ncskier commented Nov 7, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AlanGreene commented Nov 11, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ncskier commented Nov 22, 2019

Uh oh!

AlanGreene Nov 25, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ncskier Dec 5, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ncskier Dec 5, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

ncskier commented Nov 7, 2019 •

edited

Loading

AlanGreene Nov 25, 2019 •

edited

Loading

ncskier Dec 5, 2019 •

edited

Loading

ncskier Dec 5, 2019 •

edited

Loading