Added a couple of comments regarding the new changes, and spent some time
looking into the issue you mentioned about the dropdown `required` bool.
In general, Launchpad UI is set up so that we don't usually barely need to
touch JS code, which is what led me to investigate a little further into that.
I left a suggestion on how I think it should be done without having it in JS
Diff comments:
> diff --git a/lib/lp/snappy/templates/snap-edit.pt
> b/lib/lp/snappy/templates/snap-edit.pt
> index 5f911e5..e8649dc 100644
> --- a/lib/lp/snappy/templates/snap-edit.pt
> +++ b/lib/lp/snappy/templates/snap-edit.pt
> @@ -148,6 +154,47 @@
> }, window);
> });
> </script>
> +
> + <!-- Script to enable/disable the fetch_service_policy dropdown -->
> + <script type="text/javascript">
> + LPJS.use('node', function(Y) {
> + Y.on('domready', function () {
> + var checkbox = Y.one("input[name='field.use_fetch_service']");
Nit also missed in the last review: can you add more significant names to the
`checkbox` and `select` variables? These are specifically fetch service
related, so just naming it `fs_checkbox` and `fs_policy_select` or something
similar is already nicer than the generic checkbox and select
> + var select = Y.one("select[name='field.fetch_service_policy']");
> + var container = Y.one("#fetch-service-policy-container");
This isn't used anymore, can be removed
> +
> + if (!checkbox || !select || !container) return;
> +
> + var descriptionNode = container.one("p.formHelp");
I think adding details to the user is great, but this is not the right place to
update this. This is added automatically taking into account what's in the
interface file (see ISnapEditableAttributes.fetch_service_policy the field
description).
If you want to update the text, update it there as it will help both UI and API
users and doesn't require extra JS
> + if (descriptionNode) {
> + descriptionNode.setHTML("The “strict” mode only allows certain
> resources and formats, and errors out in any case the restrictions are
> violated. The “permissive” mode works similarly, but only logs a warning when
> encountering any violations.");
> + }
> +
> + function toggleFetchPolicy() {
> + var isChecked = checkbox.get('checked');
> +
> + container.setStyle('display', 'block');
Missed in the previous review: I don't think this is needed
> +
> + if (isChecked) {
> + // Remove the "(nothing selected)" option which appears
> + // because fetch_service_policy is set to 'required = False'
> + var options = select.all("option");
If we wanted to do this work around, this should be done just once at the top
of this script, not every time the toggle changes.
That being said, I spent some more time thinking about this after our chat and
I found a better way to do this.
In the BaseSnapEditView, you can add the following to the `validate_widgets()`
function so that the field is only required if we see that the
use_fetch_service is True:
```
if self.widgets.get("fetch_service_policy") is not None:
super().validate_widgets(data, ["use_fetch_service"])
self.widgets["fetch_service_policy"].context.required =
data.get("use_fetch_service")
```
You can see similar logic there for other fields. So by default, you'll need to
set the `required=True` for the fetch_service_policy field, but then add this
so that before validation we set it to the right required bool.
You can then remove this logic from the JS code, and just do something like
```
function toggleFetchPolicy() {
var showPolicyField = fs_checkbox.get('checked');
fs_select.set('disabled', !showPolicyField);
}
```
> + options.each(function(option) {
> + if (option.get('value') == "") {
> + option.remove();
> + }
> + });
> + select.set('disabled', false);
> + } else {
> + select.set('disabled', true);
> + }
> + }
> +
> + toggleFetchPolicy(); // On page load
> + checkbox.on('change', toggleFetchPolicy);
> + });
> + });
> + </script>
> </div>
>
> </body>
--
https://code.launchpad.net/~vaishnavi-asawale/launchpad/+git/launchpad-1/+merge/490259
Your team Launchpad code reviewers is requested to review the proposed merge of
~vaishnavi-asawale/launchpad/+git/launchpad-1:add-fetch-service-policy-dropdown
into launchpad:master.
_______________________________________________
Mailing list: https://launchpad.net/~launchpad-reviewers
Post to : [email protected]
Unsubscribe : https://launchpad.net/~launchpad-reviewers
More help : https://help.launchpad.net/ListHelp
