You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@trafficcontrol.apache.org by GitBox <gi...@apache.org> on 2022/03/07 22:28:50 UTC
[GitHub] [trafficcontrol] shamrickus opened a new pull request #6623: Improve TP Tenant Selection
shamrickus opened a new pull request #6623:
URL: https://github.com/apache/trafficcontrol/pull/6623
<!--
Thank you for contributing! Please be sure to read our contribution guidelines: https://github.com/apache/trafficcontrol/blob/master/CONTRIBUTING.md
If this closes or relates to an existing issue, please reference it using one of the following:
Closes: #ISSUE
Related: #ISSUE
If this PR fixes a security vulnerability, DO NOT submit! Instead, contact
the Apache Traffic Control Security Team at security@trafficcontrol.apache.org and follow the
guidelines at https://apache.org/security regarding vulnerability disclosure.
-->
This PR fixes #6427 and arguably #2980. It creates a new directive that can be used to display hierarchical data similar to tenants, and implements it for all tenant fields. It works as a replacement to the `select` element.
<!-- **^ Add meaningful description above** --><hr/>
## Which Traffic Control components are affected by this PR?
<!-- Please delete all components from this list that are NOT affected by this PR.
Feel free to add the name of a tool or script that is affected but not on the list.
-->
- Traffic Portal
## What is the best way to verify this PR?
On each of the following pages verify that validation works, and that input actually effects the relevant tenant fields:
Tenants (Add & Edit/View)
Delivery Services (All Types, Add & Edit/View)
User (Add & Edit/View)
User Registration
Origins (Add & Edit/View).
On any page verify that the icons in the drop down are expandable/collapsable, the search box implements a basic fuzzy search, and verify that the input works on all supported TP resolutions.
<!-- Please include here ALL the steps necessary to test your PR.
If your PR has tests (and most should), provide the steps needed to run the tests.
If not, please provide step-by-step instructions to test the PR manually and explain why your PR does not need tests. -->
## PR submission checklist
- [x] This PR has tests <!-- If not, please delete this text and explain why this PR does not need tests. -->
- [x] This PR has documentation <!-- If not, please delete this text and explain why this PR does not need documentation. -->
- [x] This PR has a CHANGELOG.md entry <!-- A fix for a bug from an ATC release, an improvement, or a new feature should have a changelog entry. -->
- [x] This PR **DOES NOT FIX A SERIOUS SECURITY VULNERABILITY** (see [the Apache Software Foundation's security guidelines](https://apache.org/security) for details)
<!--
Licensed to the Apache Software Foundation (ASF) under one
or more contributor license agreements. See the NOTICE file
distributed with this work for additional information
regarding copyright ownership. The ASF licenses this file
to you under the Apache License, Version 2.0 (the
"License"); you may not use this file except in compliance
with the License. You may obtain a copy of the License at
http://www.apache.org/licenses/LICENSE-2.0
Unless required by applicable law or agreed to in writing,
software distributed under the License is distributed on an
"AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
KIND, either express or implied. See the License for the
specific language governing permissions and limitations
under the License.
-->
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] shamrickus commented on a change in pull request #6623: Improve TP Tenant Selection
Posted by GitBox <gi...@apache.org>.
shamrickus commented on a change in pull request #6623:
URL: https://github.com/apache/trafficcontrol/pull/6623#discussion_r829599315
##########
File path: traffic_portal/app/src/common/modules/form/deliveryService/form.deliveryService.Steering.tpl.html
##########
@@ -155,9 +155,7 @@ <h3 ng-if="!open()">Previous Value</h3>
</div>
</label>
<div class="col-md-10 col-sm-10 col-xs-12">
- <select id="tenantId" name="tenantId" class="form-control" ng-model="deliveryService.tenantId" ng-options="tenant.id as tenantLabel(tenant) for tenant in tenants" required>
- <option disabled hidden selected value="">Select...</option>
- </select>
+ <tree-select handle="tenantId" on-update="deliveryService.tenantId=value" tree-data="[tenants[0]]" initial-value="deliveryService.tenantId"></tree-select>
Review comment:
You were correct, it should work now.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] shamrickus commented on a change in pull request #6623: Improve TP Tenant Selection
Posted by GitBox <gi...@apache.org>.
shamrickus commented on a change in pull request #6623:
URL: https://github.com/apache/trafficcontrol/pull/6623#discussion_r829566791
##########
File path: traffic_portal/app/src/common/modules/form/deliveryService/form.deliveryService.Steering.tpl.html
##########
@@ -155,9 +155,7 @@ <h3 ng-if="!open()">Previous Value</h3>
</div>
</label>
<div class="col-md-10 col-sm-10 col-xs-12">
- <select id="tenantId" name="tenantId" class="form-control" ng-model="deliveryService.tenantId" ng-options="tenant.id as tenantLabel(tenant) for tenant in tenants" required>
- <option disabled hidden selected value="">Select...</option>
- </select>
+ <tree-select handle="tenantId" on-update="deliveryService.tenantId=value" tree-data="[tenants[0]]" initial-value="deliveryService.tenantId"></tree-select>
Review comment:
That is what `on-update` does, it was a way to trigger form validation. I'll double check when I get a chance but that _should_ work.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] shamrickus commented on a change in pull request #6623: Improve TP Tenant Selection
Posted by GitBox <gi...@apache.org>.
shamrickus commented on a change in pull request #6623:
URL: https://github.com/apache/trafficcontrol/pull/6623#discussion_r838528812
##########
File path: traffic_portal/app/src/common/directives/treeSelect/TreeSelectDirective.js
##########
@@ -0,0 +1,243 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+var TreeSelectDirective = function($document) {
+ return {
+ restrict: "E",
+ require: "^form",
+ templateUrl: "common/directives/treeSelect/tree.select.tpl.html",
+ replace: true,
+ scope: {
+ treeData: '=',
+ initialValue: '<',
+ handle: '@',
+ onUpdate: "&"
+ },
+ link: function(scope, element, attrs, ngFormController) {
+ /**
+ * Non-recursed ordered list of rows to display (before filtering)
+ * @type TreeSelectDirective.RowData[]
+ */
+ scope.treeRows = [];
+ /** @type string */
+ scope.searchText = "";
+ /** @type boolean */
+ scope.shown = false;
+ /** @type TreeSelectDirective.RowData */
+ scope.selected = null;
+
+ // Bound variables
+ /** @type TreeSelectDirective.TreeData[] */
+ scope.treeData;
+ /** @type string */
+ scope.initialValue;
+ /**
+ * Used for form validation, will be assigned to an id attribute
+ * @type string
+ */
+ scope.handle;
+ /**
+ * Used to properly update the parent on value change, useful for validation.
+ * @callback onUpdate
+ * @param {string} value
+ */
+ scope.onUpdate;
Review comment:
That would not work, they're defined as strings in the scope parameter and get replaced with the true value at runtime.
I definitely would like to remove these lines is you can think of some way though.
##########
File path: traffic_portal/app/src/common/directives/treeSelect/TreeSelectDirective.js
##########
@@ -0,0 +1,243 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+var TreeSelectDirective = function($document) {
+ return {
+ restrict: "E",
+ require: "^form",
+ templateUrl: "common/directives/treeSelect/tree.select.tpl.html",
+ replace: true,
+ scope: {
+ treeData: '=',
+ initialValue: '<',
+ handle: '@',
+ onUpdate: "&"
+ },
+ link: function(scope, element, attrs, ngFormController) {
+ /**
+ * Non-recursed ordered list of rows to display (before filtering)
+ * @type TreeSelectDirective.RowData[]
+ */
+ scope.treeRows = [];
+ /** @type string */
+ scope.searchText = "";
+ /** @type boolean */
+ scope.shown = false;
+ /** @type TreeSelectDirective.RowData */
+ scope.selected = null;
+
+ // Bound variables
+ /** @type TreeSelectDirective.TreeData[] */
+ scope.treeData;
+ /** @type string */
+ scope.initialValue;
+ /**
+ * Used for form validation, will be assigned to an id attribute
+ * @type string
+ */
+ scope.handle;
+ /**
+ * Used to properly update the parent on value change, useful for validation.
+ * @callback onUpdate
+ * @param {string} value
+ */
+ scope.onUpdate;
Review comment:
That would not work, they're defined as strings in the scope parameter and get replaced with the true value at runtime.
I definitely would like to remove these lines if you can think of some way though.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] ocket8888 commented on a change in pull request #6623: Improve TP Tenant Selection
Posted by GitBox <gi...@apache.org>.
ocket8888 commented on a change in pull request #6623:
URL: https://github.com/apache/trafficcontrol/pull/6623#discussion_r821170278
##########
File path: traffic_portal/app/src/common/directives/_directives.scss
##########
@@ -0,0 +1,81 @@
+/*
+
+
+ Licensed under the Apache License, Version 2.0 (the "License");
+ you may not use this file except in compliance with the License.
+ You may obtain a copy of the License at
+
+ http://www.apache.org/licenses/LICENSE-2.0
+
+ Unless required by applicable law or agreed to in writing, software
+ distributed under the License is distributed on an "AS IS" BASIS,
+ WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ See the License for the specific language governing permissions and
+ limitations under the License.
+
+*/
+
+.has-error {
+ div.search-box {
+ label {
+ color: #555555;
+ background-color: #eeeeee;
+ border-color: #ccc;
+ }
+ input {
+ box-shadow: inset 0 1px 1px rgba(0, 0, 0, 0.075);
+ border-color: #ccc;
+ }
+ }
+}
+
+div.tree-select-root {
+ position: relative;
+
+ input.display-field {
+ background-color: #fff;
+ }
+
+ div.tree-drop-down {
+ width: 100%;
+ background: white;
+ position: absolute;
+ z-index: 2;
+ box-shadow: rgba(0, 0, 0, 0.38) 0px 3px 3px;
+ border: 1px solid #0000001f;
+ font-size: 16px;
+
+ div.search-box {
+ width: calc(100% - 10px);
+ margin: 5px auto 5px;
+
+ @media (min-width: 768px) {
+ div.helptooltip {
+ left: calc(100% - 50px);
+ }
+ }
+ }
+
+ ul.nav {
+ max-height: 550px;
+ overflow-y: scroll;
+ overflow-x: hidden;
+
+ & > li.tree-row > a {
+ padding: 5px 0 5px 10px;
+
+
+ i {
+ margin-right: 3px;
+ }
+
+ @for $i from 0 through 30 {
+ &.depth-#{$i} {
+ position: relative;
+ left: #{$i * 8}px;
+ }
+ }
Review comment:
This will break for sufficiently complex Tenancy trees - if you used a nested tree option component like
<details>
<summary><code>tree.select.tpl.html</code></summary>
```html
<div class="tree-select-root">
<input type="text" id="{{ handle }}" name="{{ handle }}" class="form-control display-field" ng-model="selected.label" ng-click="toggle()" required readonly>
<div class="tree-drop-down" ng-show="shown">
<div class="input-group search-box">
<label class="input-group-addon has-tooltip control-label" for="searchText">
Filter:
<div class="helptooltip">
<div class="helptext">Fuzzy search by tenant name</div>
</div>
</label>
<input name="searchText" type="text" class="form-control" ng-model="searchText" maxlength="48">
</div>
<ul class="nav nav-list nav-pills nav-stacked">
<treerow ng-repeat="row in treeRows | filter: checkFilters" row="row">
</treerow>
</ul>
</div>
</div>
```
</details>
<details>
<summary><code>tree.row.tpl.html</code></summary>
```html
<li>
<a ng-class="'depth-' + {{ row.depth }}" ng-click="select(row)" name="{{row.label}}">
<i class="fa" ng-class="getClass(row)" ng-click="collapse(row, $event)"></i>
<span>{{row.label}}</span>
</a>
<ul ng-if="children.length > 0">
<tree-row row="child" ng-repeat="child in children"></treerow>
</ul>
<li>
```
</details>
You could support arbitrary depths and also simply the styling a tiny bit with stacking margins
##########
File path: traffic_portal/app/src/common/directives/treeSelect/tree.select.tpl.html
##########
@@ -0,0 +1,40 @@
+<!--
+Licensed to the Apache Software Foundation (ASF) under one
+or more contributor license agreements. See the NOTICE file
+distributed with this work for additional information
+regarding copyright ownership. The ASF licenses this file
+to you under the Apache License, Version 2.0 (the
+"License"); you may not use this file except in compliance
+with the License. You may obtain a copy of the License at
+
+ http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing,
+software distributed under the License is distributed on an
+"AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+KIND, either express or implied. See the License for the
+specific language governing permissions and limitations
+under the License.
+-->
+<div class="tree-select-root">
+ <input type="text" id="{{ handle }}" name="{{ handle }}" class="form-control display-field" ng-model="selected.label" ng-click="toggle()" required readonly>
+ <div class="tree-drop-down" ng-show="shown">
+ <div class="input-group search-box">
+ <label class="input-group-addon has-tooltip control-label" for="searchText">
Review comment:
`for` must be the `id` of the labeled element - right now the `input` has a `name` but no `id`.
Also as IDs must be unique within a document, I'd recommend using the `handle` input to provide part of the ID, since that must already be unique above.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] ocket8888 commented on a change in pull request #6623: Improve TP Tenant Selection
Posted by GitBox <gi...@apache.org>.
ocket8888 commented on a change in pull request #6623:
URL: https://github.com/apache/trafficcontrol/pull/6623#discussion_r837695909
##########
File path: traffic_portal/app/src/common/directives/treeSelect/tree.select.tpl.html
##########
@@ -0,0 +1,40 @@
+<!--
+Licensed to the Apache Software Foundation (ASF) under one
+or more contributor license agreements. See the NOTICE file
+distributed with this work for additional information
+regarding copyright ownership. The ASF licenses this file
+to you under the Apache License, Version 2.0 (the
+"License"); you may not use this file except in compliance
+with the License. You may obtain a copy of the License at
+
+ http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing,
+software distributed under the License is distributed on an
+"AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+KIND, either express or implied. See the License for the
+specific language governing permissions and limitations
+under the License.
+-->
+<div class="tree-select-root">
+ <input type="text" id="{{ handle }}" name="{{ handle }}" class="form-control display-field" ng-model="selected.label" ng-click="toggle()" required readonly>
+ <div class="tree-drop-down" ng-show="shown">
+ <div class="input-group search-box">
+ <label class="input-group-addon has-tooltip control-label" for="{{ handle }}searchText">
+ Filter:
+ <div class="helptooltip">
+ <div class="helptext">Fuzzy search by tenant name</div>
+ </div>
+ </label>
+ <input id="{{ handle }}searchText" name="{{ handle }}searchText" type="search" class="form-control" ng-model="searchText" maxlength="48">
+ </div>
+ <ul class="nav nav-list nav-pills nav-stacked">
+ <li class="tree-row" ng-repeat="row in treeRows | filter: checkFilters">
+ <a ng-style="{'left': row.depth*8+'px'}" ng-click="select(row)" name="{{row.label}}">
Review comment:
Anchor elements should be links or anchors. As MDN says in [the click event handling section of the MDN docs on the Anchor element](https://developer.mozilla.org/en-US/docs/Web/HTML/Element/a#onclick_events), when you just want to handle click behavior
> Use a [`<button>`](https://developer.mozilla.org/en-US/docs/Web/HTML/Element/button) instead. In general, **you should only use a hyperlink for navigation to a real URL.**
(they put in that in bold, not me)
There may not seem to be a difference, but there are some accessibility concerns. I made this sample document:
<details><summary>Example document</summary>
```html
<html>
<head>
<title>Anchor vs Link vs Button</title>
</head>
<body style="background: white;">
<a id="anchor">My anchor</a>
<br/>
<a href="#" id="link">My link</a>
<br/>
<button id="button">My button</button>
<script>
const anchor = document.getElementById("anchor");
const link = document.getElementById("link");
const button = document.getElementById("button");
/**
* @param {Event & {target: HTMLElement}} e
*/
function clickHandler(e) {
console.log("click handler triggered for", e.target.id);
}
anchor.addEventListener("click", clickHandler);
link.addEventListener("click", clickHandler);
button.addEventListener("click", clickHandler);
document.addEventListener("keydown", e => {
if (e.key === "Tab") {
const activeElem = document.activeElement;
const identifier = activeElem.id ? activeElem.id : activeElem.tagName;
console.log("focused element is now:", identifier);
}
});
</script>
</body>
</html>
```
</details>
When you load that into a browser, you'll notice a few things. First, if you tab-cycle through the document, you'll notice that the only elements that can be focused are the `a[href]`, the `button`, and the `html` root element itself (on Firefox - in Chromium-based browsers, it curiously focuses the `body` instead). So the `a:not([href])` cannot be focused by cycling through controls. You _can_ make it focus-able by giving it a bogus `href` like `#`, but that has the unfortunate side effect of doing navigation, and since we use fragment-based AngularJS routing that's unacceptable. Setting it to `javascript:void(0)` works for preventing navigation while allowing it to be tab-able, but normal form controlling doesn't appear to work. That is, the `button` element does special coalescing on different events - if you tab to the button and hit <kbd>Space</kbd> or <kbd>Enter</kbd>, you'll see the console log line `click handler triggered for button` in the console. The anchor element wil
l coalesce the <kbd>Enter</kbd> event to also trigger a click, but it *won't* do that for <kbd>Space</kbd>.
You can see why this is in Firefox's "Acessibility" tab in its dev tools - the `a#link` element has the `link` role, while the `button` has the `pushbutton` role (which is actually the ARIA `button` role, idk why it displays the name as "pushbutton"). So you can add `role="button"` to the link - but, as stated in [the MDN docs on the "button" role](https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/Roles/button_role):
> Adding `role="button"` tells the screen reader the element is a button, but provides no button functionality.
That is to say, it still won't trigger the click event when <kbd>Space</kbd> is pressed while it's focused - in the accessibility tools you can see the problem is from the "actions" list of the emulated link/button containing only "Click" (which, combined with the "focusable" state allows the Enter->Click coalescence which you can see by the absence of that state on the `a#anchor`), while the `button` has the "Press" action.
So basically, you have to emulate the button behavior yourself. According to the Aria "button" role spec, the control has to respond to clicks, <kbd>Space</kbd> presses, and <kbd>Enter</kbd> presses. It specifies the "keydown" event (and explicitly not "keypress", which is deprecated), so you'd need to add a handler for that event that checks the key code and calls the click handler if and only if it's one of the correct codes (but actually you can rely on <kbd>Enter</kbd> to work automagically). Because links are also draggable by default, but buttons are not, it's probably best to disable that as well.
Finally, the `name` attribute of an anchor has specific semantics, and is deprecated as of HTML 5.0. In HTML 4.1 and earlier it was used to specify a target location on a page, so each of these is going to show up as a document fragment in the DOM tree and accessibility tools. I think the only reason you are adding those, though, is to make it easier to select them in the integration tests. I think you don't need to bother, since the same information is present as the text node child of the span herein contained, and since the click event will bubble upward you can select by CSS with containing text like `await element(by.cssContainingText("li.tree-row span", tenant.ParentTenant + this.randomize)).click();`.
So all told, a full implementation of `a` as a button would look like
```html
<a
ng-style="{'left': row.depth*8+'px'}"
ng-click="select(row)"
ng-keydown="handleKeydown($event, row)"
draggable="false"
href="javascript:void(0)"
>
<div ng-click="collapse(row, $event)"><i class="fa" ng-class="getClass(row)"></i></div>
<span>{{row.label}}</span>
</a>
```
where `scope.handleKeydown` is e.g.
```javascript
function handleKeydown(evt, row) {
if (evt.key === "Space") {
event.stopPropagation();
event.preventDefault();
scope.select(row);
}
}
```
Or you could use a `button[type="button"] with `appearance: none; border: none; background: transparent` and whatever else is appropriate.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] ocket8888 commented on a change in pull request #6623: Improve TP Tenant Selection
Posted by GitBox <gi...@apache.org>.
ocket8888 commented on a change in pull request #6623:
URL: https://github.com/apache/trafficcontrol/pull/6623#discussion_r837674094
##########
File path: traffic_portal/app/src/common/directives/treeSelect/TreeSelectDirective.js
##########
@@ -0,0 +1,243 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+var TreeSelectDirective = function($document) {
+ return {
+ restrict: "E",
+ require: "^form",
+ templateUrl: "common/directives/treeSelect/tree.select.tpl.html",
+ replace: true,
+ scope: {
+ treeData: '=',
+ initialValue: '<',
+ handle: '@',
+ onUpdate: "&"
+ },
+ link: function(scope, element, attrs, ngFormController) {
+ /**
+ * Non-recursed ordered list of rows to display (before filtering)
+ * @type TreeSelectDirective.RowData[]
+ */
+ scope.treeRows = [];
+ /** @type string */
+ scope.searchText = "";
+ /** @type boolean */
+ scope.shown = false;
+ /** @type TreeSelectDirective.RowData */
Review comment:
Nit: `strict`ly speaking, `null` is not assignable to `TreeSelectDirective.RowData` - actual type is `TreeSelectDirective.RowData | null`
##########
File path: traffic_portal/app/src/common/directives/treeSelect/TreeSelectDirective.js
##########
@@ -0,0 +1,243 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+var TreeSelectDirective = function($document) {
+ return {
+ restrict: "E",
+ require: "^form",
+ templateUrl: "common/directives/treeSelect/tree.select.tpl.html",
+ replace: true,
+ scope: {
+ treeData: '=',
+ initialValue: '<',
+ handle: '@',
+ onUpdate: "&"
+ },
+ link: function(scope, element, attrs, ngFormController) {
+ /**
+ * Non-recursed ordered list of rows to display (before filtering)
+ * @type TreeSelectDirective.RowData[]
+ */
+ scope.treeRows = [];
+ /** @type string */
+ scope.searchText = "";
+ /** @type boolean */
+ scope.shown = false;
+ /** @type TreeSelectDirective.RowData */
+ scope.selected = null;
+
+ // Bound variables
+ /** @type TreeSelectDirective.TreeData[] */
+ scope.treeData;
+ /** @type string */
+ scope.initialValue;
+ /**
+ * Used for form validation, will be assigned to an id attribute
+ * @type string
+ */
+ scope.handle;
+ /**
+ * Used to properly update the parent on value change, useful for validation.
+ * @callback onUpdate
+ * @param {string} value
+ */
+ scope.onUpdate;
Review comment:
I feel like maybe these should be a description on the JSDoc for the `link` function `scope` parameter, to avoid putting in actual statements that don't do anything. It's not a big deal, but I don't think the browserify/webkit stack will optimize these statements away - which probably has an imperceptible, negligible performance impact.
Change it or don't, it doesn't matter, really.
##########
File path: traffic_portal/app/src/common/directives/treeSelect/TreeSelectDirective.js
##########
@@ -0,0 +1,243 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+var TreeSelectDirective = function($document) {
+ return {
+ restrict: "E",
+ require: "^form",
+ templateUrl: "common/directives/treeSelect/tree.select.tpl.html",
+ replace: true,
+ scope: {
+ treeData: '=',
+ initialValue: '<',
+ handle: '@',
+ onUpdate: "&"
+ },
+ link: function(scope, element, attrs, ngFormController) {
+ /**
+ * Non-recursed ordered list of rows to display (before filtering)
+ * @type TreeSelectDirective.RowData[]
+ */
+ scope.treeRows = [];
+ /** @type string */
+ scope.searchText = "";
+ /** @type boolean */
+ scope.shown = false;
+ /** @type TreeSelectDirective.RowData */
+ scope.selected = null;
+
+ // Bound variables
+ /** @type TreeSelectDirective.TreeData[] */
+ scope.treeData;
+ /** @type string */
+ scope.initialValue;
+ /**
+ * Used for form validation, will be assigned to an id attribute
+ * @type string
+ */
+ scope.handle;
+ /**
+ * Used to properly update the parent on value change, useful for validation.
+ * @callback onUpdate
+ * @param {string} value
+ */
+ scope.onUpdate;
+
+
+ element.bind("click",
+ /**
+ * Handles click events within this element, prevents $document propagation as well as binds
+ * the close event to it.
+ * @param evt
+ */
+ function(evt) {
+ if(scope.shown) {
+ evt.stopPropagation();
+ $document.on("click", closeSelect);
+ }
+ });
+ /**
+ * Detects when a click is trigger outside this element. Used to close dropdown and ensure
+ * there is no $document event pollution.
+ * @param evt
+ */
+ const closeSelect = function(evt){
+ scope.close();
+ $document.off("click", closeSelect);
+ scope.$apply();
+ };
+
+ /**
+ * Initializes treeRows, must be called whenever treeData changes
+ */
+ const reInit = function() {
+ scope.treeRows = [];
+ scope.selected = null;
+ for(let data of scope.treeData) {
+ if (data != undefined)
Review comment:
Comparisons should use `===`/`!==`
##########
File path: traffic_portal/app/src/common/directives/treeSelect/tree.select.tpl.html
##########
@@ -0,0 +1,40 @@
+<!--
+Licensed to the Apache Software Foundation (ASF) under one
+or more contributor license agreements. See the NOTICE file
+distributed with this work for additional information
+regarding copyright ownership. The ASF licenses this file
+to you under the Apache License, Version 2.0 (the
+"License"); you may not use this file except in compliance
+with the License. You may obtain a copy of the License at
+
+ http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing,
+software distributed under the License is distributed on an
+"AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+KIND, either express or implied. See the License for the
+specific language governing permissions and limitations
+under the License.
+-->
+<div class="tree-select-root">
Review comment:
this should have `role="combobox"` with `aria-expanded` set to `scope.shown`
##########
File path: traffic_portal/app/src/common/directives/treeSelect/tree.select.tpl.html
##########
@@ -0,0 +1,40 @@
+<!--
+Licensed to the Apache Software Foundation (ASF) under one
+or more contributor license agreements. See the NOTICE file
+distributed with this work for additional information
+regarding copyright ownership. The ASF licenses this file
+to you under the Apache License, Version 2.0 (the
+"License"); you may not use this file except in compliance
+with the License. You may obtain a copy of the License at
+
+ http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing,
+software distributed under the License is distributed on an
+"AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+KIND, either express or implied. See the License for the
+specific language governing permissions and limitations
+under the License.
+-->
+<div class="tree-select-root">
+ <input type="text" id="{{ handle }}" name="{{ handle }}" class="form-control display-field" ng-model="selected.label" ng-click="toggle()" required readonly>
+ <div class="tree-drop-down" ng-show="shown">
+ <div class="input-group search-box">
+ <label class="input-group-addon has-tooltip control-label" for="{{ handle }}searchText">
+ Filter:
+ <div class="helptooltip">
+ <div class="helptext">Fuzzy search by tenant name</div>
+ </div>
+ </label>
+ <input id="{{ handle }}searchText" name="{{ handle }}searchText" type="search" class="form-control" ng-model="searchText" maxlength="48">
+ </div>
+ <ul class="nav nav-list nav-pills nav-stacked">
+ <li class="tree-row" ng-repeat="row in treeRows | filter: checkFilters">
+ <a ng-style="{'left': row.depth*8+'px'}" ng-click="select(row)" name="{{row.label}}">
Review comment:
Anchor elements should be links or anchors. As MDN says in [the click event handling section of the MDN docs on the Anchor element](https://developer.mozilla.org/en-US/docs/Web/HTML/Element/a#onclick_events), when you just want to handle click behavior
> Use a [`<button>`](https://developer.mozilla.org/en-US/docs/Web/HTML/Element/button) instead. In general, **you should only use a hyperlink for navigation to a real URL.**
(they put in that in bold, not me)
There may not seem to be a difference, but there are some accessibility concerns. I made this sample document:
<details><summary>Example document</summary>
```html
<html>
<head>
<title>Anchor vs Link vs Button</title>
</head>
<body style="background: white;">
<a id="anchor">My anchor</a>
<br/>
<a href="#" id="link">My link</a>
<br/>
<button id="button">My button</button>
<script>
const anchor = document.getElementById("anchor");
const link = document.getElementById("link");
const button = document.getElementById("button");
/**
* @param {Event & {target: HTMLElement}} e
*/
function clickHandler(e) {
console.log("click handler triggered for", e.target.id);
}
anchor.addEventListener("click", clickHandler);
link.addEventListener("click", clickHandler);
button.addEventListener("click", clickHandler);
document.addEventListener("keydown", e => {
if (e.key === "Tab") {
const activeElem = document.activeElement;
const identifier = activeElem.id ? activeElem.id : activeElem.tagName;
console.log("focused element is now:", identifier);
}
});
</script>
</body>
</html>
```
</details>
When you load that into a browser, you'll notice a few things. First, if you tab-cycle through the document, you'll notice that the only elements that can be focused are the `a[href]`, the `button`, and the `html` root element itself (on Firefox - in Chromium-based browsers, it curiously focuses the `body` instead). So the `a:not([href])` cannot be focused by cycling through controls. You _can_ make it focus-able by giving it a bogus `href` like `#`, but that has the unfortunate side effect of doing navigation, and since we use fragment-based AngularJS routing that's unacceptable. Setting it to `javascript:void(0)` works for preventing navigation while allowing it to be tab-able, but normal form controlling doesn't appear to work. That is, the `button` element does special coalescing on different events - if you tab to the button and hit <kbd>Space</kbd> or <kbd>Enter</kbd>, you'll see the console log line `click handler triggered for button` in the console. The anchor element wil
l coalesce the <kbd>Enter</kbd> event to also trigger a click, but it *won't* do that for <kbd>Space</kbd>.
You can see why this is in Firefox's "Acessibility" tab in its dev tools - the `a#link` element has the `link` role, while the `button` has the `pushbutton` role (which is actually the ARIA `button` role, idk why it displays the name as "pushbutton"). So you can add `role="button"` to the link - but, as stated in [the MDN docs on the "button" role](https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/Roles/button_role):
> Adding `role="button"` tells the screen reader the element is a button, but provides no button functionality.
That is to say, it still won't trigger the click event when <kbd>Space</kbd> is pressed while it's focused - in the accessibility tools you can see the problem is from the "actions" list of the emulated link/button containing only "Click" (which, combined with the "focusable" state allows the Enter->Click coalescence which you can see by the absence of that state on the `a#anchor`), while the `button` has the "Press" action.
So basically, you have to emulate the button behavior yourself. According to the Aria "button" role spec, the control has to respond to clicks, <kbd>Space</kbd> presses, and <kbd>Enter</kbd> presses. It specifies the "keydown" event (and explicitly not "keypress", which is deprecated), so you'd need to add a handler for that event that checks the key code and calls the click handler if and only if it's one of the correct codes (but actually you can rely on <kbd>Enter</kbd> to work automagically). Because links are also draggable by default, but buttons are not, it's probably best to disable that as well.
Finally, the `name` attribute of an anchor has specific semantics, and is deprecated as of HTML 5.0. In HTML 4.1 and earlier it was used to specify a target location on a page, so each of these is going to show up as a document fragment in the DOM tree and accessibility tools. I think the only reason you are adding those, though, is to make it easier to select them in the integration tests. I think you don't need to bother, since the same information is present as the text node child of the span herein contained, and since the click event will bubble upward you can select by CSS with containing text like `await element(by.cssContainingText("li.tree-row span", tenant.ParentTenant + this.randomize)).click();`.
So all told, a full implementation of `a` as a button would look like
```html
<a
ng-style="{'left': row.depth*8+'px'}"
ng-click="select(row)"
ng-keydown="handleKeydown($event, row)"
draggable="false"
href="javascript:void(0)"
>
<div ng-click="collapse(row, $event)"><i class="fa" ng-class="getClass(row)"></i></div>
<span>{{row.label}}</span>
</a>
```
where `scope.handleKeydown` is e.g.
```javascript
function handleKeydown(evt, row) {
if (evt.key === "Space") {
event.stopPropagation();
event.preventDefault();
scope.select(row);
}
}
```
Or you could use a button with `appearance: none; border: none; background: transparent`.
##########
File path: traffic_portal/app/src/common/directives/treeSelect/TreeSelectDirective.js
##########
@@ -0,0 +1,243 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+var TreeSelectDirective = function($document) {
+ return {
+ restrict: "E",
+ require: "^form",
+ templateUrl: "common/directives/treeSelect/tree.select.tpl.html",
+ replace: true,
+ scope: {
+ treeData: '=',
+ initialValue: '<',
+ handle: '@',
+ onUpdate: "&"
+ },
+ link: function(scope, element, attrs, ngFormController) {
+ /**
+ * Non-recursed ordered list of rows to display (before filtering)
+ * @type TreeSelectDirective.RowData[]
+ */
+ scope.treeRows = [];
+ /** @type string */
+ scope.searchText = "";
+ /** @type boolean */
+ scope.shown = false;
+ /** @type TreeSelectDirective.RowData */
+ scope.selected = null;
+
+ // Bound variables
+ /** @type TreeSelectDirective.TreeData[] */
+ scope.treeData;
+ /** @type string */
+ scope.initialValue;
+ /**
+ * Used for form validation, will be assigned to an id attribute
+ * @type string
+ */
+ scope.handle;
+ /**
+ * Used to properly update the parent on value change, useful for validation.
+ * @callback onUpdate
+ * @param {string} value
+ */
+ scope.onUpdate;
+
+
+ element.bind("click",
+ /**
+ * Handles click events within this element, prevents $document propagation as well as binds
+ * the close event to it.
+ * @param evt
+ */
+ function(evt) {
+ if(scope.shown) {
+ evt.stopPropagation();
+ $document.on("click", closeSelect);
+ }
+ });
+ /**
+ * Detects when a click is trigger outside this element. Used to close dropdown and ensure
+ * there is no $document event pollution.
+ * @param evt
+ */
+ const closeSelect = function(evt){
+ scope.close();
+ $document.off("click", closeSelect);
+ scope.$apply();
+ };
+
+ /**
+ * Initializes treeRows, must be called whenever treeData changes
+ */
+ const reInit = function() {
+ scope.treeRows = [];
+ scope.selected = null;
+ for(let data of scope.treeData) {
+ if (data != undefined)
+ addNode(data, 0);
+ }
+ }
+
+ /**
+ * Converts a tree data node into row data recursively.
+ * @param {TreeSelectDirective.TreeData} row
+ * @param {number} depth
+ * @returns TreeSelectDirective.RowData
+ */
+ const addNode = function(row, depth) {
+ scope.treeRows.push({
+ label: row.name,
+ value: row.id,
+ depth: depth,
+ children: [],
+ collapsed: false,
+ hidden: false
+ });
+ const last = scope.treeRows.length - 1;
+ if(row.id === scope.initialValue || row.name === scope.initialValue) {
+ scope.selected = scope.treeRows[last];
+ }
+ if(row.children != null) {
Review comment:
Comparisons should use `===`/`!==`
##########
File path: traffic_portal/app/src/common/directives/treeSelect/TreeSelectDirective.js
##########
@@ -0,0 +1,243 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+var TreeSelectDirective = function($document) {
+ return {
+ restrict: "E",
+ require: "^form",
+ templateUrl: "common/directives/treeSelect/tree.select.tpl.html",
+ replace: true,
+ scope: {
+ treeData: '=',
+ initialValue: '<',
+ handle: '@',
+ onUpdate: "&"
+ },
+ link: function(scope, element, attrs, ngFormController) {
+ /**
+ * Non-recursed ordered list of rows to display (before filtering)
+ * @type TreeSelectDirective.RowData[]
+ */
+ scope.treeRows = [];
+ /** @type string */
+ scope.searchText = "";
+ /** @type boolean */
+ scope.shown = false;
+ /** @type TreeSelectDirective.RowData */
+ scope.selected = null;
+
+ // Bound variables
+ /** @type TreeSelectDirective.TreeData[] */
+ scope.treeData;
+ /** @type string */
+ scope.initialValue;
+ /**
+ * Used for form validation, will be assigned to an id attribute
+ * @type string
+ */
+ scope.handle;
+ /**
+ * Used to properly update the parent on value change, useful for validation.
+ * @callback onUpdate
+ * @param {string} value
+ */
+ scope.onUpdate;
+
+
+ element.bind("click",
+ /**
+ * Handles click events within this element, prevents $document propagation as well as binds
+ * the close event to it.
+ * @param evt
+ */
+ function(evt) {
+ if(scope.shown) {
+ evt.stopPropagation();
+ $document.on("click", closeSelect);
+ }
+ });
+ /**
+ * Detects when a click is trigger outside this element. Used to close dropdown and ensure
+ * there is no $document event pollution.
+ * @param evt
+ */
+ const closeSelect = function(evt){
+ scope.close();
+ $document.off("click", closeSelect);
+ scope.$apply();
+ };
+
+ /**
+ * Initializes treeRows, must be called whenever treeData changes
+ */
+ const reInit = function() {
+ scope.treeRows = [];
+ scope.selected = null;
+ for(let data of scope.treeData) {
+ if (data != undefined)
+ addNode(data, 0);
+ }
+ }
+
+ /**
+ * Converts a tree data node into row data recursively.
+ * @param {TreeSelectDirective.TreeData} row
+ * @param {number} depth
+ * @returns TreeSelectDirective.RowData
+ */
+ const addNode = function(row, depth) {
+ scope.treeRows.push({
Review comment:
nit: If you just assign this object to a variable before pushing it into the scope's tree rows, you can use that reference instead of doing `scope.treeRows[last]` everywhere
##########
File path: traffic_portal/app/src/common/directives/treeSelect/TreeSelectDirective.js
##########
@@ -0,0 +1,243 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+var TreeSelectDirective = function($document) {
+ return {
+ restrict: "E",
+ require: "^form",
+ templateUrl: "common/directives/treeSelect/tree.select.tpl.html",
+ replace: true,
+ scope: {
+ treeData: '=',
+ initialValue: '<',
+ handle: '@',
+ onUpdate: "&"
+ },
+ link: function(scope, element, attrs, ngFormController) {
+ /**
+ * Non-recursed ordered list of rows to display (before filtering)
+ * @type TreeSelectDirective.RowData[]
+ */
+ scope.treeRows = [];
+ /** @type string */
+ scope.searchText = "";
+ /** @type boolean */
+ scope.shown = false;
+ /** @type TreeSelectDirective.RowData */
+ scope.selected = null;
+
+ // Bound variables
+ /** @type TreeSelectDirective.TreeData[] */
+ scope.treeData;
+ /** @type string */
+ scope.initialValue;
+ /**
+ * Used for form validation, will be assigned to an id attribute
+ * @type string
+ */
+ scope.handle;
+ /**
+ * Used to properly update the parent on value change, useful for validation.
+ * @callback onUpdate
+ * @param {string} value
+ */
+ scope.onUpdate;
+
+
+ element.bind("click",
+ /**
+ * Handles click events within this element, prevents $document propagation as well as binds
+ * the close event to it.
+ * @param evt
+ */
+ function(evt) {
+ if(scope.shown) {
+ evt.stopPropagation();
+ $document.on("click", closeSelect);
+ }
+ });
+ /**
+ * Detects when a click is trigger outside this element. Used to close dropdown and ensure
+ * there is no $document event pollution.
+ * @param evt
+ */
+ const closeSelect = function(evt){
+ scope.close();
+ $document.off("click", closeSelect);
+ scope.$apply();
+ };
+
+ /**
+ * Initializes treeRows, must be called whenever treeData changes
+ */
+ const reInit = function() {
+ scope.treeRows = [];
+ scope.selected = null;
+ for(let data of scope.treeData) {
+ if (data != undefined)
+ addNode(data, 0);
+ }
+ }
+
+ /**
+ * Converts a tree data node into row data recursively.
+ * @param {TreeSelectDirective.TreeData} row
+ * @param {number} depth
+ * @returns TreeSelectDirective.RowData
+ */
+ const addNode = function(row, depth) {
+ scope.treeRows.push({
+ label: row.name,
+ value: row.id,
+ depth: depth,
+ children: [],
+ collapsed: false,
+ hidden: false
+ });
+ const last = scope.treeRows.length - 1;
+ if(row.id === scope.initialValue || row.name === scope.initialValue) {
+ scope.selected = scope.treeRows[last];
+ }
+ if(row.children != null) {
+ for(const child of row.children) {
+ if(child === undefined) continue;
+ scope.treeRows[last].children.push(addNode(child, depth + 1));
+ }
+ }
+ return scope.treeRows[last];
+ }
+
+ /**
+ * Collapses a row data and recursively hides and collapses its children.
+ * @param {TreeSelectDirective.RowData} row
+ * @param {boolean?} state
+ */
+ const collapseRecurse = function(row, state) {
+ if(row.children.length === 0) return;
+ for(const treeRow of scope.treeRows) {
+ if (treeRow.value === row.value) {
+ if(state === undefined)
+ treeRow.collapsed = !treeRow.collapsed;
+ else
+ treeRow.collapsed = state;
+ for(let treeChild of treeRow.children) {
Review comment:
nit: `treeChild` is never reassigned. Could be `const` instead
##########
File path: traffic_portal/app/src/common/directives/treeSelect/TreeSelectDirective.js
##########
@@ -0,0 +1,243 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+var TreeSelectDirective = function($document) {
+ return {
+ restrict: "E",
+ require: "^form",
+ templateUrl: "common/directives/treeSelect/tree.select.tpl.html",
+ replace: true,
+ scope: {
+ treeData: '=',
+ initialValue: '<',
+ handle: '@',
+ onUpdate: "&"
+ },
+ link: function(scope, element, attrs, ngFormController) {
+ /**
+ * Non-recursed ordered list of rows to display (before filtering)
+ * @type TreeSelectDirective.RowData[]
+ */
+ scope.treeRows = [];
+ /** @type string */
+ scope.searchText = "";
+ /** @type boolean */
+ scope.shown = false;
+ /** @type TreeSelectDirective.RowData */
+ scope.selected = null;
+
+ // Bound variables
+ /** @type TreeSelectDirective.TreeData[] */
+ scope.treeData;
+ /** @type string */
+ scope.initialValue;
+ /**
+ * Used for form validation, will be assigned to an id attribute
+ * @type string
+ */
+ scope.handle;
+ /**
+ * Used to properly update the parent on value change, useful for validation.
+ * @callback onUpdate
+ * @param {string} value
+ */
+ scope.onUpdate;
+
+
+ element.bind("click",
+ /**
+ * Handles click events within this element, prevents $document propagation as well as binds
+ * the close event to it.
+ * @param evt
+ */
+ function(evt) {
+ if(scope.shown) {
+ evt.stopPropagation();
+ $document.on("click", closeSelect);
+ }
+ });
+ /**
+ * Detects when a click is trigger outside this element. Used to close dropdown and ensure
+ * there is no $document event pollution.
+ * @param evt
+ */
+ const closeSelect = function(evt){
+ scope.close();
+ $document.off("click", closeSelect);
+ scope.$apply();
+ };
+
+ /**
+ * Initializes treeRows, must be called whenever treeData changes
+ */
+ const reInit = function() {
+ scope.treeRows = [];
+ scope.selected = null;
+ for(let data of scope.treeData) {
+ if (data != undefined)
+ addNode(data, 0);
+ }
+ }
+
+ /**
+ * Converts a tree data node into row data recursively.
+ * @param {TreeSelectDirective.TreeData} row
+ * @param {number} depth
+ * @returns TreeSelectDirective.RowData
+ */
+ const addNode = function(row, depth) {
+ scope.treeRows.push({
+ label: row.name,
+ value: row.id,
+ depth: depth,
+ children: [],
+ collapsed: false,
+ hidden: false
+ });
+ const last = scope.treeRows.length - 1;
+ if(row.id === scope.initialValue || row.name === scope.initialValue) {
+ scope.selected = scope.treeRows[last];
+ }
+ if(row.children != null) {
+ for(const child of row.children) {
+ if(child === undefined) continue;
+ scope.treeRows[last].children.push(addNode(child, depth + 1));
+ }
+ }
+ return scope.treeRows[last];
+ }
+
+ /**
+ * Collapses a row data and recursively hides and collapses its children.
+ * @param {TreeSelectDirective.RowData} row
+ * @param {boolean?} state
+ */
+ const collapseRecurse = function(row, state) {
+ if(row.children.length === 0) return;
+ for(const treeRow of scope.treeRows) {
+ if (treeRow.value === row.value) {
+ if(state === undefined)
+ treeRow.collapsed = !treeRow.collapsed;
+ else
+ treeRow.collapsed = state;
+ for(let treeChild of treeRow.children) {
+ treeChild.hidden = treeRow.collapsed;
+ collapseRecurse(treeChild, treeRow.collapsed);
+ }
+ }
+ }
+ }
+
+ /**
+ * Returns true if the inputs letters are also present in text with the same order
+ * @param {string} text
+ * @param {string} input
+ * @returns {boolean}
+ */
+ const fuzzyMatch = function(text, input) {
+ if(input === "") return true;
+ if(text === undefined) return false;
+ text = text.toString().toLowerCase();
+ input = input.toString().toLowerCase();
+ let n = -1;
+ for(let i in input) {
+ const letter = input[i];
+ if (!~(n = text.indexOf(letter, n + 1))) return false;
+ }
+ return true;
+ }
+
+ /**
+ * Triggers onUpdate binding
+ */
+ scope.update = function() {
+ scope.onUpdate({value: scope.selected.value ?? ""});
+ }
+
+ /**
+ * Toggle the dropdown menu
+ */
+ scope.toggle = function() {
+ scope.shown = !scope.shown;
+ }
+ /**
+ * Close the dropdown menu.
+ */
+ scope.close = function() {
+ scope.shown = false;
+ }
+
+
+ /**
+ * Updates the selection when clicking a dropdown option
+ * @param {TreeSelectDirective.RowData} row
+ */
+ scope.select = function(row) {
+ scope.selected = row;
+ scope.selection = row.value;
+ scope.update();
+
+ if(row.value !== this.initialValue) {
+ ngFormController[this.handle].$setDirty();
+ } else {
+ ngFormController[this.handle].$setPristine();
+ }
Review comment:
Is this proper? According to [the AngularJS `form.FormController` docs](https://docs.angularjs.org/api/ng/type/form.FormController#$pristine):
> **$pristine**
> True if user has not interacted with the form yet.<br/>
> **$dirty**
> True if user has already interacted with the form.
Since this is in the select event handler, the user must be interacting with the form, so shouldn't it always be "dirty" in that case? Angular (2+) has a separate concept on its `FormControl` for "touched" vs "untouched", where it says "dirty" means
> ... the user has changed the value in the UI.
and "touched" means
> ... the user has triggered a blur event on it [the form control].
but AngularJS makes no distinction between "touching" a control and "dirtying" it afaik
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] ocket8888 commented on a change in pull request #6623: Improve TP Tenant Selection
Posted by GitBox <gi...@apache.org>.
ocket8888 commented on a change in pull request #6623:
URL: https://github.com/apache/trafficcontrol/pull/6623#discussion_r837695909
##########
File path: traffic_portal/app/src/common/directives/treeSelect/tree.select.tpl.html
##########
@@ -0,0 +1,40 @@
+<!--
+Licensed to the Apache Software Foundation (ASF) under one
+or more contributor license agreements. See the NOTICE file
+distributed with this work for additional information
+regarding copyright ownership. The ASF licenses this file
+to you under the Apache License, Version 2.0 (the
+"License"); you may not use this file except in compliance
+with the License. You may obtain a copy of the License at
+
+ http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing,
+software distributed under the License is distributed on an
+"AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+KIND, either express or implied. See the License for the
+specific language governing permissions and limitations
+under the License.
+-->
+<div class="tree-select-root">
+ <input type="text" id="{{ handle }}" name="{{ handle }}" class="form-control display-field" ng-model="selected.label" ng-click="toggle()" required readonly>
+ <div class="tree-drop-down" ng-show="shown">
+ <div class="input-group search-box">
+ <label class="input-group-addon has-tooltip control-label" for="{{ handle }}searchText">
+ Filter:
+ <div class="helptooltip">
+ <div class="helptext">Fuzzy search by tenant name</div>
+ </div>
+ </label>
+ <input id="{{ handle }}searchText" name="{{ handle }}searchText" type="search" class="form-control" ng-model="searchText" maxlength="48">
+ </div>
+ <ul class="nav nav-list nav-pills nav-stacked">
+ <li class="tree-row" ng-repeat="row in treeRows | filter: checkFilters">
+ <a ng-style="{'left': row.depth*8+'px'}" ng-click="select(row)" name="{{row.label}}">
Review comment:
Anchor elements should be links or anchors. As MDN says in [the click event handling section of the MDN docs on the Anchor element](https://developer.mozilla.org/en-US/docs/Web/HTML/Element/a#onclick_events), when you just want to handle click behavior
> Use a [`<button>`](https://developer.mozilla.org/en-US/docs/Web/HTML/Element/button) instead. In general, **you should only use a hyperlink for navigation to a real URL.**
(they put in that in bold, not me)
There may not seem to be a difference, but there are some accessibility concerns. I made this sample document:
<details><summary>Example document</summary>
```html
<html>
<head>
<title>Anchor vs Link vs Button</title>
</head>
<body style="background: white;">
<a id="anchor">My anchor</a>
<br/>
<a href="#" id="link">My link</a>
<br/>
<button id="button">My button</button>
<script>
const anchor = document.getElementById("anchor");
const link = document.getElementById("link");
const button = document.getElementById("button");
/**
* @param {Event & {target: HTMLElement}} e
*/
function clickHandler(e) {
console.log("click handler triggered for", e.target.id);
}
anchor.addEventListener("click", clickHandler);
link.addEventListener("click", clickHandler);
button.addEventListener("click", clickHandler);
document.addEventListener("keydown", e => {
if (e.key === "Tab") {
const activeElem = document.activeElement;
const identifier = activeElem.id ? activeElem.id : activeElem.tagName;
console.log("focused element is now:", identifier);
}
});
</script>
</body>
</html>
```
</details>
When you load that into a browser, you'll notice a few things. First, if you tab-cycle through the document, you'll notice that the only elements that can be focused are the `a[href]`, the `button`, and the `html` root element itself (on Firefox - in Chromium-based browsers, it curiously focuses the `body` instead). So the `a:not([href])` cannot be focused by cycling through controls. You _can_ make it focus-able by giving it a bogus `href` like `#`, but that has the unfortunate side effect of doing navigation, and since we use fragment-based AngularJS routing that's unacceptable. Setting it to `javascript:void(0)` works for preventing navigation while allowing it to be tab-able, but normal form controlling doesn't appear to work. That is, the `button` element does special coalescing on different events - if you tab to the button and hit <kbd>Space</kbd> or <kbd>Enter</kbd>, you'll see the console log line `click handler triggered for button` in the console. The anchor element wil
l coalesce the <kbd>Enter</kbd> event to also trigger a click, but it *won't* do that for <kbd>Space</kbd>.
You can see why this is in Firefox's "Acessibility" tab in its dev tools - the `a#link` element has the `link` role, while the `button` has the `pushbutton` role (which is actually the ARIA `button` role, idk why it displays the name as "pushbutton"). So you can add `role="button"` to the link - but, as stated in [the MDN docs on the "button" role](https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/Roles/button_role):
> Adding `role="button"` tells the screen reader the element is a button, but provides no button functionality.
That is to say, it still won't trigger the click event when <kbd>Space</kbd> is pressed while it's focused - in the accessibility tools you can see the problem is from the "actions" list of the emulated link/button containing only "Click" (which, combined with the "focusable" state allows the Enter->Click coalescence which you can see by the absence of that state on the `a#anchor`), while the `button` has the "Press" action.
So basically, you have to emulate the button behavior yourself. According to the Aria "button" role spec, the control has to respond to clicks, <kbd>Space</kbd> presses, and <kbd>Enter</kbd> presses. It specifies the "keydown" event (and explicitly not "keypress", which is deprecated), so you'd need to add a handler for that event that checks the key code and calls the click handler if and only if it's one of the correct codes (but actually you can rely on <kbd>Enter</kbd> to work automagically). Because links are also draggable by default, but buttons are not, it's probably best to disable that as well.
Finally, the `name` attribute of an anchor has specific semantics, and is deprecated as of HTML 5.0. In HTML 4.1 and earlier it was used to specify a target location on a page, so each of these is going to show up as a document fragment in the DOM tree and accessibility tools. I think the only reason you are adding those, though, is to make it easier to select them in the integration tests. I think you don't need to bother, since the same information is present as the text node child of the span herein contained, and since the click event will bubble upward you can select by CSS with containing text like `await element(by.cssContainingText("li.tree-row span", tenant.ParentTenant + this.randomize)).click();`.
So all told, a full implementation of `a` as a button would look like
```html
<a
ng-style="{'left': row.depth*8+'px'}"
ng-click="select(row)"
ng-keydown="handleKeydown($event, row)"
draggable="false"
href="javascript:void(0)"
>
<div ng-click="collapse(row, $event)"><i class="fa" ng-class="getClass(row)"></i></div>
<span>{{row.label}}</span>
</a>
```
where `scope.handleKeydown` is e.g.
```javascript
function handleKeydown(evt, row) {
if (evt.key === " ") {
event.stopPropagation();
event.preventDefault();
scope.select(row);
}
}
```
Or you could use a `button[type="button"] with `appearance: none; border: none; background: transparent` and whatever else is appropriate.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] ocket8888 commented on a change in pull request #6623: Improve TP Tenant Selection
Posted by GitBox <gi...@apache.org>.
ocket8888 commented on a change in pull request #6623:
URL: https://github.com/apache/trafficcontrol/pull/6623#discussion_r829553410
##########
File path: traffic_portal/app/src/common/directives/treeSelect/TreeSelectDirective.js
##########
@@ -0,0 +1,236 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+var TreeSelectDirective = function($document) {
+ return {
+ restrict: "E",
+ templateUrl: "common/directives/treeSelect/tree.select.tpl.html",
+ replace: true,
+ scope: {
+ treeData: '=',
+ initialValue: '<',
+ handle: '@',
+ onUpdate: "&"
+ },
+ link: function(scope, element, attrs) {
+ /**
+ * Non-recursed ordered list of rows to display (before filtering)
+ * @type [TreeSelectDirective.RowData]
Review comment:
Tuple is used here, should probably be an array (esp. given the below assignment would actually fail compilation if it were enforced).
##########
File path: traffic_portal/app/src/common/directives/treeSelect/TreeSelectDirective.d.ts
##########
@@ -0,0 +1,34 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+export namespace TreeSelectDirective {
+ export interface TreeData {
+ name: string;
+ id: string;
+ children: [TreeData];
Review comment:
This specifies a tuple containing exactly one `TreeData`. I think it should be `TreeData[]` or `Array<TreeData>` (I tend to prefer the latter because working with Go so much gets me confused which side the `[]` goes on if I do it the reverse way in TS - but it truly does not matter which you want to use).
##########
File path: traffic_portal/app/src/common/directives/treeSelect/tree.select.tpl.html
##########
@@ -0,0 +1,40 @@
+<!--
+Licensed to the Apache Software Foundation (ASF) under one
+or more contributor license agreements. See the NOTICE file
+distributed with this work for additional information
+regarding copyright ownership. The ASF licenses this file
+to you under the Apache License, Version 2.0 (the
+"License"); you may not use this file except in compliance
+with the License. You may obtain a copy of the License at
+
+ http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing,
+software distributed under the License is distributed on an
+"AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+KIND, either express or implied. See the License for the
+specific language governing permissions and limitations
+under the License.
+-->
+<div class="tree-select-root">
+ <input type="text" id="{{ handle }}" name="{{ handle }}" class="form-control display-field" ng-model="selected.label" ng-click="toggle()" required readonly>
+ <div class="tree-drop-down" ng-show="shown">
+ <div class="input-group search-box">
+ <label class="input-group-addon has-tooltip control-label" for="{{ handle }}searchText">
+ Filter:
+ <div class="helptooltip">
+ <div class="helptext">Fuzzy search by tenant name</div>
+ </div>
+ </label>
+ <input id="{{ handle }}searchText" name="{{ handle }}searchText" type="text" class="form-control" ng-model="searchText" maxlength="48">
Review comment:
search inputs should use an `input[type="search"]`
##########
File path: traffic_portal/app/src/common/directives/treeSelect/TreeSelectDirective.d.ts
##########
@@ -0,0 +1,34 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+export namespace TreeSelectDirective {
+ export interface TreeData {
+ name: string;
+ id: string;
+ children: [TreeData];
+ }
+ export interface RowData {
+ label: string;
+ value: string;
+ depth: number;
+ collapsed: boolean;
+ hidden: boolean;
+ children: [RowData];
Review comment:
Same as above RE: Array vs tuple typing
##########
File path: traffic_portal/app/src/common/modules/form/deliveryService/form.deliveryService.Steering.tpl.html
##########
@@ -155,9 +155,7 @@ <h3 ng-if="!open()">Previous Value</h3>
</div>
</label>
<div class="col-md-10 col-sm-10 col-xs-12">
- <select id="tenantId" name="tenantId" class="form-control" ng-model="deliveryService.tenantId" ng-options="tenant.id as tenantLabel(tenant) for tenant in tenants" required>
- <option disabled hidden selected value="">Select...</option>
- </select>
+ <tree-select handle="tenantId" on-update="deliveryService.tenantId=value" tree-data="[tenants[0]]" initial-value="deliveryService.tenantId"></tree-select>
Review comment:
Because this no longer uses `ngModel`, it doesn't cause the form to be marked dirty; this means that one can no longer change the tenant of a Delivery Service. At least, not without making any other changes to it so that the form is dirty.
I believe that applies to everywhere this is being used - I only checked the DS and user pages, but that's sufficient to convince me it's systemic.
##########
File path: traffic_portal/app/src/common/directives/treeSelect/TreeSelectDirective.js
##########
@@ -0,0 +1,236 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+var TreeSelectDirective = function($document) {
+ return {
+ restrict: "E",
+ templateUrl: "common/directives/treeSelect/tree.select.tpl.html",
+ replace: true,
+ scope: {
+ treeData: '=',
+ initialValue: '<',
+ handle: '@',
+ onUpdate: "&"
+ },
+ link: function(scope, element, attrs) {
+ /**
+ * Non-recursed ordered list of rows to display (before filtering)
+ * @type [TreeSelectDirective.RowData]
+ */
+ scope.treeRows = [];
+ /** @type string */
+ scope.searchText = "";
+ /** @type boolean */
+ scope.shown = false;
+ /** @type TreeSelectDirective.RowData */
+ scope.selected = null;
+
+ // Bound variables
+ /** @type [TreeSelectDirective.TreeData] */
Review comment:
Tuple is used here, should probably be an array (esp. given the below assignment would actually fail compilation if it were enforced).
##########
File path: traffic_portal/app/src/common/directives/treeSelect/TreeSelectDirective.js
##########
@@ -0,0 +1,236 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+var TreeSelectDirective = function($document) {
+ return {
+ restrict: "E",
+ templateUrl: "common/directives/treeSelect/tree.select.tpl.html",
+ replace: true,
+ scope: {
+ treeData: '=',
+ initialValue: '<',
+ handle: '@',
+ onUpdate: "&"
+ },
+ link: function(scope, element, attrs) {
+ /**
+ * Non-recursed ordered list of rows to display (before filtering)
+ * @type [TreeSelectDirective.RowData]
+ */
+ scope.treeRows = [];
+ /** @type string */
+ scope.searchText = "";
+ /** @type boolean */
+ scope.shown = false;
+ /** @type TreeSelectDirective.RowData */
+ scope.selected = null;
+
+ // Bound variables
+ /** @type [TreeSelectDirective.TreeData] */
+ scope.treeData;
+ /** @type string */
+ scope.initialValue;
+ /**
+ * Used for form validation, will be assigned to an id attribute
+ * @type string
+ */
+ scope.handle;
+ /**
+ * Used to properly update the parent on value change, useful for validation.
+ * @callback onUpdate
+ * @param {string} value
+ */
+ scope.onUpdate;
+
+
+ element.bind("click",
+ /**
+ * Handles click events within this element, prevents $document propagation as well as binds
+ * the close event to it.
+ * @param evt
+ */
+ function(evt) {
+ if(scope.shown) {
+ evt.stopPropagation();
+ $document.on("click", closeSelect);
+ }
+ });
+ /**
+ * Detects when a click is trigger outside this element. Used to close dropdown and ensure
+ * there is no $document event pollution.
+ * @param evt
+ */
+ const closeSelect = function(evt){
+ scope.close();
+ $document.off("click", closeSelect);
+ scope.$apply();
+ };
+
+ /**
+ * Initializes treeRows, must be called whenever treeData changes
+ */
+ const reInit = function() {
+ scope.treeRows = [];
+ scope.selected = null;
+ for(let data of scope.treeData) {
Review comment:
nit: `data` is never reassigned. Could be `const` instead.
##########
File path: traffic_portal/test/integration/Data/deliveryservices.ts
##########
@@ -18,316 +18,319 @@
*/
export const deliveryservices = {
cleanup: [
- {
- action: "DeleteServers",
- route: "/servers",
- method: "delete",
- data: [
- {
- route: "/servers/",
- getRequest: [
- {
- route: "/servers",
- queryKey: "hostName",
- queryValue: "DSTest",
- replace: "route"
- }
- ]
- }
- ]
- },
Review comment:
You changed the indentation in this file from glorious true indentation to hateful spaces-as-indentation. Makes it hard to pick out the changes. And also tab > space.
##########
File path: traffic_portal/app/src/common/directives/treeSelect/TreeSelectDirective.js
##########
@@ -0,0 +1,236 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+var TreeSelectDirective = function($document) {
+ return {
+ restrict: "E",
+ templateUrl: "common/directives/treeSelect/tree.select.tpl.html",
+ replace: true,
+ scope: {
+ treeData: '=',
+ initialValue: '<',
+ handle: '@',
+ onUpdate: "&"
+ },
+ link: function(scope, element, attrs) {
+ /**
+ * Non-recursed ordered list of rows to display (before filtering)
+ * @type [TreeSelectDirective.RowData]
+ */
+ scope.treeRows = [];
+ /** @type string */
+ scope.searchText = "";
+ /** @type boolean */
+ scope.shown = false;
+ /** @type TreeSelectDirective.RowData */
+ scope.selected = null;
+
+ // Bound variables
+ /** @type [TreeSelectDirective.TreeData] */
+ scope.treeData;
+ /** @type string */
+ scope.initialValue;
+ /**
+ * Used for form validation, will be assigned to an id attribute
+ * @type string
+ */
+ scope.handle;
+ /**
+ * Used to properly update the parent on value change, useful for validation.
+ * @callback onUpdate
+ * @param {string} value
+ */
+ scope.onUpdate;
+
+
+ element.bind("click",
+ /**
+ * Handles click events within this element, prevents $document propagation as well as binds
+ * the close event to it.
+ * @param evt
+ */
+ function(evt) {
+ if(scope.shown) {
+ evt.stopPropagation();
+ $document.on("click", closeSelect);
+ }
+ });
+ /**
+ * Detects when a click is trigger outside this element. Used to close dropdown and ensure
+ * there is no $document event pollution.
+ * @param evt
+ */
+ const closeSelect = function(evt){
+ scope.close();
+ $document.off("click", closeSelect);
+ scope.$apply();
+ };
+
+ /**
+ * Initializes treeRows, must be called whenever treeData changes
+ */
+ const reInit = function() {
+ scope.treeRows = [];
+ scope.selected = null;
+ for(let data of scope.treeData) {
+ if (data != undefined)
Review comment:
Comparisons should use `===`/`!==`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] ocket8888 merged pull request #6623: Improve TP Tenant Selection
Posted by GitBox <gi...@apache.org>.
ocket8888 merged pull request #6623:
URL: https://github.com/apache/trafficcontrol/pull/6623
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] ocket8888 commented on a change in pull request #6623: Improve TP Tenant Selection
Posted by GitBox <gi...@apache.org>.
ocket8888 commented on a change in pull request #6623:
URL: https://github.com/apache/trafficcontrol/pull/6623#discussion_r840090300
##########
File path: traffic_portal/app/src/common/directives/treeSelect/TreeSelectDirective.js
##########
@@ -0,0 +1,243 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+var TreeSelectDirective = function($document) {
+ return {
+ restrict: "E",
+ require: "^form",
+ templateUrl: "common/directives/treeSelect/tree.select.tpl.html",
+ replace: true,
+ scope: {
+ treeData: '=',
+ initialValue: '<',
+ handle: '@',
+ onUpdate: "&"
+ },
+ link: function(scope, element, attrs, ngFormController) {
+ /**
+ * Non-recursed ordered list of rows to display (before filtering)
+ * @type TreeSelectDirective.RowData[]
+ */
+ scope.treeRows = [];
+ /** @type string */
+ scope.searchText = "";
+ /** @type boolean */
+ scope.shown = false;
+ /** @type TreeSelectDirective.RowData */
+ scope.selected = null;
+
+ // Bound variables
+ /** @type TreeSelectDirective.TreeData[] */
+ scope.treeData;
+ /** @type string */
+ scope.initialValue;
+ /**
+ * Used for form validation, will be assigned to an id attribute
+ * @type string
+ */
+ scope.handle;
+ /**
+ * Used to properly update the parent on value change, useful for validation.
+ * @callback onUpdate
+ * @param {string} value
+ */
+ scope.onUpdate;
Review comment:
Originally what I meant was squeeze the entire type of `scope` into an `@param` on `link`, but I think it's actually way easier to add another interface to your `.d.ts` file. I have a PR (https://github.com/shamrickus/trafficcontrol/pull/1) where I fixed typings throughout the file so **_nothing_** has an implicit `any` type.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] shamrickus commented on a change in pull request #6623: Improve TP Tenant Selection
Posted by GitBox <gi...@apache.org>.
shamrickus commented on a change in pull request #6623:
URL: https://github.com/apache/trafficcontrol/pull/6623#discussion_r821177509
##########
File path: traffic_portal/app/src/common/directives/_directives.scss
##########
@@ -0,0 +1,81 @@
+/*
+
+
+ Licensed under the Apache License, Version 2.0 (the "License");
+ you may not use this file except in compliance with the License.
+ You may obtain a copy of the License at
+
+ http://www.apache.org/licenses/LICENSE-2.0
+
+ Unless required by applicable law or agreed to in writing, software
+ distributed under the License is distributed on an "AS IS" BASIS,
+ WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ See the License for the specific language governing permissions and
+ limitations under the License.
+
+*/
+
+.has-error {
+ div.search-box {
+ label {
+ color: #555555;
+ background-color: #eeeeee;
+ border-color: #ccc;
+ }
+ input {
+ box-shadow: inset 0 1px 1px rgba(0, 0, 0, 0.075);
+ border-color: #ccc;
+ }
+ }
+}
+
+div.tree-select-root {
+ position: relative;
+
+ input.display-field {
+ background-color: #fff;
+ }
+
+ div.tree-drop-down {
+ width: 100%;
+ background: white;
+ position: absolute;
+ z-index: 2;
+ box-shadow: rgba(0, 0, 0, 0.38) 0px 3px 3px;
+ border: 1px solid #0000001f;
+ font-size: 16px;
+
+ div.search-box {
+ width: calc(100% - 10px);
+ margin: 5px auto 5px;
+
+ @media (min-width: 768px) {
+ div.helptooltip {
+ left: calc(100% - 50px);
+ }
+ }
+ }
+
+ ul.nav {
+ max-height: 550px;
+ overflow-y: scroll;
+ overflow-x: hidden;
+
+ & > li.tree-row > a {
+ padding: 5px 0 5px 10px;
+
+
+ i {
+ margin-right: 3px;
+ }
+
+ @for $i from 0 through 30 {
+ &.depth-#{$i} {
+ position: relative;
+ left: #{$i * 8}px;
+ }
+ }
Review comment:
I'd really rather avoid having to make another directive/component (which is the only way to do that in ajs). I'll move the `left` portion of the css to angular in an `ng-style`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] shamrickus commented on a change in pull request #6623: Improve TP Tenant Selection
Posted by GitBox <gi...@apache.org>.
shamrickus commented on a change in pull request #6623:
URL: https://github.com/apache/trafficcontrol/pull/6623#discussion_r838563613
##########
File path: traffic_portal/app/src/common/directives/treeSelect/TreeSelectDirective.js
##########
@@ -0,0 +1,243 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+var TreeSelectDirective = function($document) {
+ return {
+ restrict: "E",
+ require: "^form",
+ templateUrl: "common/directives/treeSelect/tree.select.tpl.html",
+ replace: true,
+ scope: {
+ treeData: '=',
+ initialValue: '<',
+ handle: '@',
+ onUpdate: "&"
+ },
+ link: function(scope, element, attrs, ngFormController) {
+ /**
+ * Non-recursed ordered list of rows to display (before filtering)
+ * @type TreeSelectDirective.RowData[]
+ */
+ scope.treeRows = [];
+ /** @type string */
+ scope.searchText = "";
+ /** @type boolean */
+ scope.shown = false;
+ /** @type TreeSelectDirective.RowData */
+ scope.selected = null;
+
+ // Bound variables
+ /** @type TreeSelectDirective.TreeData[] */
+ scope.treeData;
+ /** @type string */
+ scope.initialValue;
+ /**
+ * Used for form validation, will be assigned to an id attribute
+ * @type string
+ */
+ scope.handle;
+ /**
+ * Used to properly update the parent on value change, useful for validation.
+ * @callback onUpdate
+ * @param {string} value
+ */
+ scope.onUpdate;
+
+
+ element.bind("click",
+ /**
+ * Handles click events within this element, prevents $document propagation as well as binds
+ * the close event to it.
+ * @param evt
+ */
+ function(evt) {
+ if(scope.shown) {
+ evt.stopPropagation();
+ $document.on("click", closeSelect);
+ }
+ });
+ /**
+ * Detects when a click is trigger outside this element. Used to close dropdown and ensure
+ * there is no $document event pollution.
+ * @param evt
+ */
+ const closeSelect = function(evt){
+ scope.close();
+ $document.off("click", closeSelect);
+ scope.$apply();
+ };
+
+ /**
+ * Initializes treeRows, must be called whenever treeData changes
+ */
+ const reInit = function() {
+ scope.treeRows = [];
+ scope.selected = null;
+ for(let data of scope.treeData) {
+ if (data != undefined)
+ addNode(data, 0);
+ }
+ }
+
+ /**
+ * Converts a tree data node into row data recursively.
+ * @param {TreeSelectDirective.TreeData} row
+ * @param {number} depth
+ * @returns TreeSelectDirective.RowData
+ */
+ const addNode = function(row, depth) {
+ scope.treeRows.push({
+ label: row.name,
+ value: row.id,
+ depth: depth,
+ children: [],
+ collapsed: false,
+ hidden: false
+ });
+ const last = scope.treeRows.length - 1;
+ if(row.id === scope.initialValue || row.name === scope.initialValue) {
+ scope.selected = scope.treeRows[last];
+ }
+ if(row.children != null) {
+ for(const child of row.children) {
+ if(child === undefined) continue;
+ scope.treeRows[last].children.push(addNode(child, depth + 1));
+ }
+ }
+ return scope.treeRows[last];
+ }
+
+ /**
+ * Collapses a row data and recursively hides and collapses its children.
+ * @param {TreeSelectDirective.RowData} row
+ * @param {boolean?} state
+ */
+ const collapseRecurse = function(row, state) {
+ if(row.children.length === 0) return;
+ for(const treeRow of scope.treeRows) {
+ if (treeRow.value === row.value) {
+ if(state === undefined)
+ treeRow.collapsed = !treeRow.collapsed;
+ else
+ treeRow.collapsed = state;
+ for(let treeChild of treeRow.children) {
+ treeChild.hidden = treeRow.collapsed;
+ collapseRecurse(treeChild, treeRow.collapsed);
+ }
+ }
+ }
+ }
+
+ /**
+ * Returns true if the inputs letters are also present in text with the same order
+ * @param {string} text
+ * @param {string} input
+ * @returns {boolean}
+ */
+ const fuzzyMatch = function(text, input) {
+ if(input === "") return true;
+ if(text === undefined) return false;
+ text = text.toString().toLowerCase();
+ input = input.toString().toLowerCase();
+ let n = -1;
+ for(let i in input) {
+ const letter = input[i];
+ if (!~(n = text.indexOf(letter, n + 1))) return false;
+ }
+ return true;
+ }
+
+ /**
+ * Triggers onUpdate binding
+ */
+ scope.update = function() {
+ scope.onUpdate({value: scope.selected.value ?? ""});
+ }
+
+ /**
+ * Toggle the dropdown menu
+ */
+ scope.toggle = function() {
+ scope.shown = !scope.shown;
+ }
+ /**
+ * Close the dropdown menu.
+ */
+ scope.close = function() {
+ scope.shown = false;
+ }
+
+
+ /**
+ * Updates the selection when clicking a dropdown option
+ * @param {TreeSelectDirective.RowData} row
+ */
+ scope.select = function(row) {
+ scope.selected = row;
+ scope.selection = row.value;
+ scope.update();
+
+ if(row.value !== this.initialValue) {
+ ngFormController[this.handle].$setDirty();
+ } else {
+ ngFormController[this.handle].$setPristine();
+ }
Review comment:
It is not, I've removed the `$setDirty` as this is already handled.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] ocket8888 commented on a change in pull request #6623: Improve TP Tenant Selection
Posted by GitBox <gi...@apache.org>.
ocket8888 commented on a change in pull request #6623:
URL: https://github.com/apache/trafficcontrol/pull/6623#discussion_r829553410
##########
File path: traffic_portal/app/src/common/directives/treeSelect/TreeSelectDirective.js
##########
@@ -0,0 +1,236 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+var TreeSelectDirective = function($document) {
+ return {
+ restrict: "E",
+ templateUrl: "common/directives/treeSelect/tree.select.tpl.html",
+ replace: true,
+ scope: {
+ treeData: '=',
+ initialValue: '<',
+ handle: '@',
+ onUpdate: "&"
+ },
+ link: function(scope, element, attrs) {
+ /**
+ * Non-recursed ordered list of rows to display (before filtering)
+ * @type [TreeSelectDirective.RowData]
Review comment:
Tuple is used here, should probably be an array (esp. given the below assignment would actually fail compilation if it were enforced).
##########
File path: traffic_portal/app/src/common/directives/treeSelect/TreeSelectDirective.d.ts
##########
@@ -0,0 +1,34 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+export namespace TreeSelectDirective {
+ export interface TreeData {
+ name: string;
+ id: string;
+ children: [TreeData];
Review comment:
This specifies a tuple containing exactly one `TreeData`. I think it should be `TreeData[]` or `Array<TreeData>` (I tend to prefer the latter because working with Go so much gets me confused which side the `[]` goes on if I do it the reverse way in TS - but it truly does not matter which you want to use).
##########
File path: traffic_portal/app/src/common/directives/treeSelect/tree.select.tpl.html
##########
@@ -0,0 +1,40 @@
+<!--
+Licensed to the Apache Software Foundation (ASF) under one
+or more contributor license agreements. See the NOTICE file
+distributed with this work for additional information
+regarding copyright ownership. The ASF licenses this file
+to you under the Apache License, Version 2.0 (the
+"License"); you may not use this file except in compliance
+with the License. You may obtain a copy of the License at
+
+ http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing,
+software distributed under the License is distributed on an
+"AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+KIND, either express or implied. See the License for the
+specific language governing permissions and limitations
+under the License.
+-->
+<div class="tree-select-root">
+ <input type="text" id="{{ handle }}" name="{{ handle }}" class="form-control display-field" ng-model="selected.label" ng-click="toggle()" required readonly>
+ <div class="tree-drop-down" ng-show="shown">
+ <div class="input-group search-box">
+ <label class="input-group-addon has-tooltip control-label" for="{{ handle }}searchText">
+ Filter:
+ <div class="helptooltip">
+ <div class="helptext">Fuzzy search by tenant name</div>
+ </div>
+ </label>
+ <input id="{{ handle }}searchText" name="{{ handle }}searchText" type="text" class="form-control" ng-model="searchText" maxlength="48">
Review comment:
search inputs should use an `input[type="search"]`
##########
File path: traffic_portal/app/src/common/directives/treeSelect/TreeSelectDirective.d.ts
##########
@@ -0,0 +1,34 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+export namespace TreeSelectDirective {
+ export interface TreeData {
+ name: string;
+ id: string;
+ children: [TreeData];
+ }
+ export interface RowData {
+ label: string;
+ value: string;
+ depth: number;
+ collapsed: boolean;
+ hidden: boolean;
+ children: [RowData];
Review comment:
Same as above RE: Array vs tuple typing
##########
File path: traffic_portal/app/src/common/modules/form/deliveryService/form.deliveryService.Steering.tpl.html
##########
@@ -155,9 +155,7 @@ <h3 ng-if="!open()">Previous Value</h3>
</div>
</label>
<div class="col-md-10 col-sm-10 col-xs-12">
- <select id="tenantId" name="tenantId" class="form-control" ng-model="deliveryService.tenantId" ng-options="tenant.id as tenantLabel(tenant) for tenant in tenants" required>
- <option disabled hidden selected value="">Select...</option>
- </select>
+ <tree-select handle="tenantId" on-update="deliveryService.tenantId=value" tree-data="[tenants[0]]" initial-value="deliveryService.tenantId"></tree-select>
Review comment:
Because this no longer uses `ngModel`, it doesn't cause the form to be marked dirty; this means that one can no longer change the tenant of a Delivery Service. At least, not without making any other changes to it so that the form is dirty.
I believe that applies to everywhere this is being used - I only checked the DS and user pages, but that's sufficient to convince me it's systemic.
##########
File path: traffic_portal/app/src/common/directives/treeSelect/TreeSelectDirective.js
##########
@@ -0,0 +1,236 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+var TreeSelectDirective = function($document) {
+ return {
+ restrict: "E",
+ templateUrl: "common/directives/treeSelect/tree.select.tpl.html",
+ replace: true,
+ scope: {
+ treeData: '=',
+ initialValue: '<',
+ handle: '@',
+ onUpdate: "&"
+ },
+ link: function(scope, element, attrs) {
+ /**
+ * Non-recursed ordered list of rows to display (before filtering)
+ * @type [TreeSelectDirective.RowData]
+ */
+ scope.treeRows = [];
+ /** @type string */
+ scope.searchText = "";
+ /** @type boolean */
+ scope.shown = false;
+ /** @type TreeSelectDirective.RowData */
+ scope.selected = null;
+
+ // Bound variables
+ /** @type [TreeSelectDirective.TreeData] */
Review comment:
Tuple is used here, should probably be an array (esp. given the below assignment would actually fail compilation if it were enforced).
##########
File path: traffic_portal/app/src/common/directives/treeSelect/TreeSelectDirective.js
##########
@@ -0,0 +1,236 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+var TreeSelectDirective = function($document) {
+ return {
+ restrict: "E",
+ templateUrl: "common/directives/treeSelect/tree.select.tpl.html",
+ replace: true,
+ scope: {
+ treeData: '=',
+ initialValue: '<',
+ handle: '@',
+ onUpdate: "&"
+ },
+ link: function(scope, element, attrs) {
+ /**
+ * Non-recursed ordered list of rows to display (before filtering)
+ * @type [TreeSelectDirective.RowData]
+ */
+ scope.treeRows = [];
+ /** @type string */
+ scope.searchText = "";
+ /** @type boolean */
+ scope.shown = false;
+ /** @type TreeSelectDirective.RowData */
+ scope.selected = null;
+
+ // Bound variables
+ /** @type [TreeSelectDirective.TreeData] */
+ scope.treeData;
+ /** @type string */
+ scope.initialValue;
+ /**
+ * Used for form validation, will be assigned to an id attribute
+ * @type string
+ */
+ scope.handle;
+ /**
+ * Used to properly update the parent on value change, useful for validation.
+ * @callback onUpdate
+ * @param {string} value
+ */
+ scope.onUpdate;
+
+
+ element.bind("click",
+ /**
+ * Handles click events within this element, prevents $document propagation as well as binds
+ * the close event to it.
+ * @param evt
+ */
+ function(evt) {
+ if(scope.shown) {
+ evt.stopPropagation();
+ $document.on("click", closeSelect);
+ }
+ });
+ /**
+ * Detects when a click is trigger outside this element. Used to close dropdown and ensure
+ * there is no $document event pollution.
+ * @param evt
+ */
+ const closeSelect = function(evt){
+ scope.close();
+ $document.off("click", closeSelect);
+ scope.$apply();
+ };
+
+ /**
+ * Initializes treeRows, must be called whenever treeData changes
+ */
+ const reInit = function() {
+ scope.treeRows = [];
+ scope.selected = null;
+ for(let data of scope.treeData) {
Review comment:
nit: `data` is never reassigned. Could be `const` instead.
##########
File path: traffic_portal/test/integration/Data/deliveryservices.ts
##########
@@ -18,316 +18,319 @@
*/
export const deliveryservices = {
cleanup: [
- {
- action: "DeleteServers",
- route: "/servers",
- method: "delete",
- data: [
- {
- route: "/servers/",
- getRequest: [
- {
- route: "/servers",
- queryKey: "hostName",
- queryValue: "DSTest",
- replace: "route"
- }
- ]
- }
- ]
- },
Review comment:
You changed the indentation in this file from glorious true indentation to hateful spaces-as-indentation. Makes it hard to pick out the changes. And also tab > space.
##########
File path: traffic_portal/app/src/common/directives/treeSelect/TreeSelectDirective.js
##########
@@ -0,0 +1,236 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+var TreeSelectDirective = function($document) {
+ return {
+ restrict: "E",
+ templateUrl: "common/directives/treeSelect/tree.select.tpl.html",
+ replace: true,
+ scope: {
+ treeData: '=',
+ initialValue: '<',
+ handle: '@',
+ onUpdate: "&"
+ },
+ link: function(scope, element, attrs) {
+ /**
+ * Non-recursed ordered list of rows to display (before filtering)
+ * @type [TreeSelectDirective.RowData]
+ */
+ scope.treeRows = [];
+ /** @type string */
+ scope.searchText = "";
+ /** @type boolean */
+ scope.shown = false;
+ /** @type TreeSelectDirective.RowData */
+ scope.selected = null;
+
+ // Bound variables
+ /** @type [TreeSelectDirective.TreeData] */
+ scope.treeData;
+ /** @type string */
+ scope.initialValue;
+ /**
+ * Used for form validation, will be assigned to an id attribute
+ * @type string
+ */
+ scope.handle;
+ /**
+ * Used to properly update the parent on value change, useful for validation.
+ * @callback onUpdate
+ * @param {string} value
+ */
+ scope.onUpdate;
+
+
+ element.bind("click",
+ /**
+ * Handles click events within this element, prevents $document propagation as well as binds
+ * the close event to it.
+ * @param evt
+ */
+ function(evt) {
+ if(scope.shown) {
+ evt.stopPropagation();
+ $document.on("click", closeSelect);
+ }
+ });
+ /**
+ * Detects when a click is trigger outside this element. Used to close dropdown and ensure
+ * there is no $document event pollution.
+ * @param evt
+ */
+ const closeSelect = function(evt){
+ scope.close();
+ $document.off("click", closeSelect);
+ scope.$apply();
+ };
+
+ /**
+ * Initializes treeRows, must be called whenever treeData changes
+ */
+ const reInit = function() {
+ scope.treeRows = [];
+ scope.selected = null;
+ for(let data of scope.treeData) {
+ if (data != undefined)
Review comment:
Comparisons should use `===`/`!==`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] shamrickus commented on a change in pull request #6623: Improve TP Tenant Selection
Posted by GitBox <gi...@apache.org>.
shamrickus commented on a change in pull request #6623:
URL: https://github.com/apache/trafficcontrol/pull/6623#discussion_r829566791
##########
File path: traffic_portal/app/src/common/modules/form/deliveryService/form.deliveryService.Steering.tpl.html
##########
@@ -155,9 +155,7 @@ <h3 ng-if="!open()">Previous Value</h3>
</div>
</label>
<div class="col-md-10 col-sm-10 col-xs-12">
- <select id="tenantId" name="tenantId" class="form-control" ng-model="deliveryService.tenantId" ng-options="tenant.id as tenantLabel(tenant) for tenant in tenants" required>
- <option disabled hidden selected value="">Select...</option>
- </select>
+ <tree-select handle="tenantId" on-update="deliveryService.tenantId=value" tree-data="[tenants[0]]" initial-value="deliveryService.tenantId"></tree-select>
Review comment:
That is what `on-update` does, it was a way to trigger form validation. I'll double check when I get a chance but that _should_ work.
##########
File path: traffic_portal/app/src/common/modules/form/deliveryService/form.deliveryService.Steering.tpl.html
##########
@@ -155,9 +155,7 @@ <h3 ng-if="!open()">Previous Value</h3>
</div>
</label>
<div class="col-md-10 col-sm-10 col-xs-12">
- <select id="tenantId" name="tenantId" class="form-control" ng-model="deliveryService.tenantId" ng-options="tenant.id as tenantLabel(tenant) for tenant in tenants" required>
- <option disabled hidden selected value="">Select...</option>
- </select>
+ <tree-select handle="tenantId" on-update="deliveryService.tenantId=value" tree-data="[tenants[0]]" initial-value="deliveryService.tenantId"></tree-select>
Review comment:
You were correct, it should work now.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] shamrickus commented on a change in pull request #6623: Improve TP Tenant Selection
Posted by GitBox <gi...@apache.org>.
shamrickus commented on a change in pull request #6623:
URL: https://github.com/apache/trafficcontrol/pull/6623#discussion_r838528812
##########
File path: traffic_portal/app/src/common/directives/treeSelect/TreeSelectDirective.js
##########
@@ -0,0 +1,243 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+var TreeSelectDirective = function($document) {
+ return {
+ restrict: "E",
+ require: "^form",
+ templateUrl: "common/directives/treeSelect/tree.select.tpl.html",
+ replace: true,
+ scope: {
+ treeData: '=',
+ initialValue: '<',
+ handle: '@',
+ onUpdate: "&"
+ },
+ link: function(scope, element, attrs, ngFormController) {
+ /**
+ * Non-recursed ordered list of rows to display (before filtering)
+ * @type TreeSelectDirective.RowData[]
+ */
+ scope.treeRows = [];
+ /** @type string */
+ scope.searchText = "";
+ /** @type boolean */
+ scope.shown = false;
+ /** @type TreeSelectDirective.RowData */
+ scope.selected = null;
+
+ // Bound variables
+ /** @type TreeSelectDirective.TreeData[] */
+ scope.treeData;
+ /** @type string */
+ scope.initialValue;
+ /**
+ * Used for form validation, will be assigned to an id attribute
+ * @type string
+ */
+ scope.handle;
+ /**
+ * Used to properly update the parent on value change, useful for validation.
+ * @callback onUpdate
+ * @param {string} value
+ */
+ scope.onUpdate;
Review comment:
That would not work, they're defined as strings in the scope parameter and get replaced with the true value at runtime.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org