Rough User Interface for Creating and Listing Webhooks #52

dibbles · 2019-06-05T14:06:20Z

Changes

User interface for the webhooks extension - admittedly this is quite rough but better to start with something and fix it up.

Submitter Checklist

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

Includes docs (if user facing)
[] Commit messages follow commit message best practices

See the contribution guide
for more details.

dibbles · 2019-06-05T14:08:56Z

@jessm12 please look at this ..... I know it all needs squashing and bringing up to date etc etc... but would be good to have a look over for glaring issues right now.

I'm still working on the tests and also have this list of toDos (minimum) ....

Architecture decision, should namespace menu on left affect the web hook display table
Spinner on table load should be table skeleton
Carbon guidelines / Accessibility etc
Change validation to be on click of create, use red boxes (Webhook)
- Validate display name chars (steal from main dashboard)
- Validate URL entry (steal from main dashboard)
- Validate docker registry field ?
Change validation to be on click of create, use red boxes (Access Token)
Fix password area (couple of characters behind eye)
Tab navigation around panels
Better add button or just Icon
Namespace should not be query parameter on POST
Close button on notifications

jessm12 · 2019-06-06T09:47:17Z

webhooks-extension/src/components/WebhookDisplayTable/WebhookDisplayTable.js

+  formatCellContent(id, value) {
+    // Render the git repo as a clickable link
+    if (id.endsWith(":repository")) {
+      return <a href={value} target="_blank">{value}</a>


Use the Link router component with to instead of href here

I think though am not 100% certain that react's Link is for internal navigation only?? Not going to waste time investigating heavily at this point in time - would also need to work out how to open in new window.

jessm12 · 2019-06-06T10:04:19Z

webhooks-extension/src/components/WebhookDisplayTable/WebhookDisplayTable.js

+            key: 'namespace',
+            header: 'Namespace',
+          }
+        ];


Are these keys and headers necessary when the header and key are the same, could you use just a list of header names?

The header and key are subtly different ... there is capitalisation in the Header and in one case spacing ... "Git Repository" ..... the 'key' value is matched in the setting up of initialRows .... so im currently inclined to keep it as it is.

jessm12 · 2019-06-06T10:14:21Z

webhooks-extension/src/components/WebhookCreate/WebhookCreate.scss

@@ -0,0 +1,185 @@
+.webhook-create {
+


Just a couple of comments about styling, where possible rem is probably preferable over px, when it's 0 this can be just on its own. Also, there are some carbon preset colours that could be useful if there are colours we like available

some modifications made

jessm12 · 2019-06-06T10:15:06Z

webhooks-extension/src/components/WebhookCreate/WebhookCreate.scss

+    align-items: center;
+    display: flex;
+  }
+


Might be able to reduce some duplication of styles here, for example the only difference between these two is the visibility

@extend added to numerous elements

jessm12 · 2019-06-06T10:34:43Z

webhooks-extension/src/components/WebhookCreate/WebhookCreate.js

+    };
+  }
+
+  async fetchNamespaces() {


Can these fetch/get methods be extracted into a different file to be used by the component? I think that could make the code easier to read and would reduce the amount of code in this one file, though I do see they are altering state so might not be an option currently

I'd rather any refactor was done later ... we likely need to look at whether we could/should introduce any redux usage which might further alter things

jessm12 · 2019-06-06T10:37:03Z

webhooks-extension/src/WebhookApp.js

+    super(props);
+    this.state = {
+      // Used to show webhook created notification on table display
+      showNotification: false,


ShowNotification is a general variable, can it be more specific so it doesn't require comment

changing to ShowNotificationOnTable

jessm12 · 2019-06-06T10:38:53Z

webhooks-extension/src/components/WebhookCreate/WebhookCreate.js

+      this.fetchPipelines(itemText.selectedItem);
+    }
+    this.fetchSecrets(itemText.selectedItem);
+    this.fetchServiceAccounts(itemText.selectedItem);


Should you be handling the rerender loop here as you are with the fetchPipelines?

jessm12 · 2019-06-06T10:44:52Z

webhooks-extension/src/components/WebhookCreate/WebhookCreate.js

+
+  handleChangeServiceAcct = (itemText, value) => {
+    this.setState({serviceAccount: itemText.selectedItem });
+  }


You should be able to bring these three methods into one method by just substituting the state name, gitsecret etc, also value is not used here

An intial user interface for creating and listing webhooks, this includes the ability to click a link to take you to the git repo. A delete button is provided but the implementation will follow under a separate PR. Tests are also included.

jessm12 · 2019-06-13T12:28:51Z

/lgtm
/approve

/meow dream

tekton-robot · 2019-06-13T12:28:53Z

@jessm12:

In response to this:

/lgtm
/approve

/meow dream

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.

a-roberts · 2019-06-13T12:30:07Z

/approve

vdemeester

/meow boxes

tekton-robot · 2019-06-13T12:32:42Z

@vdemeester:

In response to this:

/meow boxes

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.

tekton-robot · 2019-06-13T12:32:45Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: a-roberts, dibbles, jessm12, vdemeester

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:

~~OWNERS~~ [vdemeester]

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 akihikokuroda and vtereso June 5, 2019 14:06

tekton-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Jun 5, 2019

a-roberts requested review from jessm12 and removed request for akihikokuroda and vtereso June 5, 2019 14:09

jessm12 suggested changes Jun 6, 2019

View reviewed changes

mnuttall mentioned this pull request Jun 7, 2019

'Getting started' guide for webhooks #54

Closed

dibbles force-pushed the ui2 branch 4 times, most recently from 0aaaac4 to 036ada5 Compare June 13, 2019 09:19

dibbles changed the title ~~DO NOT MERGE Work In Progress - Rough User Interface~~ Rough User Interface for Creating and Listing Webhooks Jun 13, 2019

dibbles force-pushed the ui2 branch from 036ada5 to ecef548 Compare June 13, 2019 10:54

dibbles force-pushed the ui2 branch from ecef548 to 45d7d87 Compare June 13, 2019 12:18

tekton-robot assigned jessm12 Jun 13, 2019

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

vdemeester approved these changes Jun 13, 2019

View reviewed changes

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

tekton-robot merged commit a149f04 into tektoncd:master Jun 13, 2019

dibbles deleted the ui2 branch June 28, 2019 11:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rough User Interface for Creating and Listing Webhooks #52

Rough User Interface for Creating and Listing Webhooks #52

dibbles commented Jun 5, 2019

dibbles commented Jun 5, 2019

jessm12 Jun 6, 2019 •

edited

dibbles Jun 11, 2019

jessm12 Jun 6, 2019

dibbles Jun 11, 2019

jessm12 Jun 6, 2019

dibbles Jun 11, 2019

jessm12 Jun 6, 2019

dibbles Jun 11, 2019

jessm12 Jun 6, 2019

dibbles Jun 11, 2019

jessm12 Jun 6, 2019

dibbles Jun 11, 2019

jessm12 Jun 6, 2019

jessm12 Jun 6, 2019

jessm12 commented Jun 13, 2019

tekton-robot commented Jun 13, 2019

a-roberts commented Jun 13, 2019

vdemeester left a comment

tekton-robot commented Jun 13, 2019

tekton-robot commented Jun 13, 2019

Rough User Interface for Creating and Listing Webhooks #52

Rough User Interface for Creating and Listing Webhooks #52

Conversation

dibbles commented Jun 5, 2019

Changes

Submitter Checklist

dibbles commented Jun 5, 2019

jessm12 Jun 6, 2019 • edited

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

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

jessm12 commented Jun 13, 2019

tekton-robot commented Jun 13, 2019

a-roberts commented Jun 13, 2019

vdemeester left a comment

Choose a reason for hiding this comment

tekton-robot commented Jun 13, 2019

tekton-robot commented Jun 13, 2019

jessm12 Jun 6, 2019 •

edited