You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@trafficcontrol.apache.org by "ocket8888 (via GitHub)" <gi...@apache.org> on 2023/01/26 22:40:50 UTC
[GitHub] [trafficcontrol] ocket8888 commented on a diff in pull request #7314: Add capability to Delivery Service (HTTP, DNS)
ocket8888 commented on code in PR #7314:
URL: https://github.com/apache/trafficcontrol/pull/7314#discussion_r1088391722
##########
traffic_portal/app/src/common/modules/form/deliveryService/form.deliveryService.DNS.tpl.html:
##########
@@ -343,6 +342,34 @@ <h3 ng-if="!open()">Previous Value</h3>
</div>
</div>
</div>
+ <div class="form-group" ng-class="{'has-error': hasError(generalConfig.requiredCapabilities), 'has-feedback': hasError(generalConfig.requiredCapabilities)}">
+ <label class="has-tooltip control-label col-md-2 col-sm-2 col-xs-12" for="capability">Required Capability
+ <div class="helptooltip">
+ <div class="helptext">Required capability(ies) associated with a delivery service</div>
+ </div>
+ </label>
+ <div class="col-md-10 col-sm-10 col-xs-12">
+ <div class="checkbox-scroll-container">
+ <div ng-repeat="capability in requiredCapabilities">
+ <input
+ type="checkbox"
+ id="capability"
+ name="capability"
+ ng-model="selectedCapabilities[capability.name]"
+ ng-checked="selectedCapabilities[capability.name]"
+ />
+ <label>
+ <a ng-href="/#!/server-capabilities/edit?name={{capability.name}}" target="_blank"> {{capability.name}}</a>
+ </label>
Review Comment:
From [the MDN documentation on the label element](https://developer.mozilla.org/en-US/docs/Web/HTML/Element/label):
> Don't place interactive elements such as [anchors](https://github.com/apache/trafficcontrol/pull/7314/a.html) or [buttons](https://github.com/apache/trafficcontrol/pull/7314/button.html) inside a label. Doing so makes it difficult for people to activate the form input associated with the label.
By design, a label is associated with a form control. Clicking on the label is intended to be identical to clicking on the form control - but that can't be the case if it has a different behavior on click. Furthermore, in order to be associated with an input, every label must either contain the element it labels or have a [`for` attribute](https://developer.mozilla.org/en-US/docs/Web/HTML/Element/label#attr-for) set to the ID of the element it labels.
##########
traffic_portal/app/src/common/modules/form/deliveryService/form.deliveryService.DNS.tpl.html:
##########
@@ -343,6 +342,34 @@ <h3 ng-if="!open()">Previous Value</h3>
</div>
</div>
</div>
+ <div class="form-group" ng-class="{'has-error': hasError(generalConfig.requiredCapabilities), 'has-feedback': hasError(generalConfig.requiredCapabilities)}">
+ <label class="has-tooltip control-label col-md-2 col-sm-2 col-xs-12" for="capability">Required Capability
+ <div class="helptooltip">
+ <div class="helptext">Required capability(ies) associated with a delivery service</div>
+ </div>
+ </label>
+ <div class="col-md-10 col-sm-10 col-xs-12">
+ <div class="checkbox-scroll-container">
+ <div ng-repeat="capability in requiredCapabilities">
+ <input
+ type="checkbox"
+ id="capability"
Review Comment:
element IDs must be unique, this has the same ID for every inserted checkbox.
##########
traffic_portal/app/src/common/modules/form/deliveryService/form.deliveryService.DNS.tpl.html:
##########
@@ -343,6 +342,34 @@ <h3 ng-if="!open()">Previous Value</h3>
</div>
</div>
</div>
+ <div class="form-group" ng-class="{'has-error': hasError(generalConfig.requiredCapabilities), 'has-feedback': hasError(generalConfig.requiredCapabilities)}">
+ <label class="has-tooltip control-label col-md-2 col-sm-2 col-xs-12" for="capability">Required Capability
+ <div class="helptooltip">
+ <div class="helptext">Required capability(ies) associated with a delivery service</div>
Review Comment:
"Delivery Service" should be capitalized and there's an extra space between those words. Also "a" should probably be "this".
##########
traffic_portal/app/src/common/modules/form/_form.scss:
##########
@@ -370,3 +370,10 @@ input:checked + .slider::before {
text-shadow: none;
}
}
+
+.checkbox-scroll-container {
+ max-height: 3.5em;
+ overflow-y: auto;
+ display: grid;
+ grid-template-columns: 1fr 1fr 1fr 1fr 1fr 1fr 1fr;
Review Comment:
I know this grid was my idea, but thinking more about it, this looks fine on desktops, but it'll be weird on mobile. The rest of the form is responsive to screen size, but this is always 8 columns, even when that doesn't fit. So instead of a grid, it's probably best to just flex. Given this HTML:
```html
<div class="checkbox-scroll-container">
<div ng-repeat="capability in requiredCapabilities">
<input
type="checkbox"
id="capability"
name="capability"
ng-checked="selectedCapabilities[capability.name]"
/>
<label>
<a ng-href="/#!/server-capabilities/edit?name={{capability.name}}" target="_blank"> {{capability.name}}</a>
</label>
</div>
</div>
```
I wrote this SCSS
```scss
.checkbox-scroll-container {
max-height: 3.5em;
overflow-y: auto;
display: flex;
flex-wrap: wrap;
justify-content: space-between;
align-content: space-between;
div {
min-width: 5em;
width: fit-content;
}
}
```
which does pretty much exactly what we want. It'll space the controls evenly in the space it has and if it runs out it'll just wrap to the next line. It doesn't guarantee any number of capabilities on a single line, because the size of the control depends on the size of the name of the capability. Should work fine on mobile as well as it does on desktops.
##########
traffic_portal/app/src/common/modules/form/deliveryService/form.deliveryService.DNS.tpl.html:
##########
@@ -343,6 +342,34 @@ <h3 ng-if="!open()">Previous Value</h3>
</div>
</div>
</div>
+ <div class="form-group" ng-class="{'has-error': hasError(generalConfig.requiredCapabilities), 'has-feedback': hasError(generalConfig.requiredCapabilities)}">
+ <label class="has-tooltip control-label col-md-2 col-sm-2 col-xs-12" for="capability">Required Capability
Review Comment:
labels can only be associated with one control, and this one is currently associated with every checkbox for DSRCs. I think what you really wanna do here is make a section of the form and add a section label for it, which is best accomplished by `fieldset` and `legend`, respectively.
##########
traffic_portal/app/src/common/modules/form/_form.scss:
##########
@@ -370,3 +370,10 @@ input:checked + .slider::before {
text-shadow: none;
}
}
+
+.checkbox-scroll-container {
+ max-height: 3.5em;
+ overflow-y: auto;
+ display: grid;
+ grid-template-columns: 1fr 1fr 1fr 1fr 1fr 1fr 1fr;
Review Comment:
inconsistent indention here
--
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