Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

LTD-1345: Select advice approval, refusal, footnotes text from picklists #319

Merged
merged 4 commits into from Dec 6, 2021

Conversation

saruniitr
Copy link
Contributor

Change description

Add option to select advice approval, refusal, footnote text from picklists (list of user defined text blocks).

Approve, Refuse advice forms are updated to use picklists

…m picklists

Picklists are predefined text for each category.
@saruniitr saruniitr force-pushed the LTD-1345-Select-advice-text-from-picklists branch from 1a1ad79 to 7b35ad1 Compare December 2, 2021 15:16

{% block javascript %}
<script nonce="{{ request.csp_nonce }}">
$("a[target={{ target }}]").click(function () {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When the link on the form is clicked, this gets called and brings up the Modal to select a picklist item

Copy link
Contributor

@alixedi alixedi Dec 2, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@saruniitr Might be better to have a single js function defined separately and call it with the target and picklist_name as an argument here. Something like -

$("a[target={{ target }}]").click(picklist({{ target }}, {{ picklist_name }}))

Whereas, separately -

def picklist(target, picklist_name) {
    LITECommon.Modal.....
}

Copy link
Contributor Author

@saruniitr saruniitr Dec 2, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@alixedi seeing some issues with that,

    $("a[target={{ target }}]").click(pickList(target));

    function pickList(target) {
        debugger
        LITECommon.Modal.showModal('Select a {{ picklist_name }} picklist item', $('#{{ target }}-picklist-picker').html(), false, true, { maxWidth: '600px' });
        filter{{ target }}PicklistItems(true);
        $(".lite-modal .app-picklist__search-input").on('input propertychange paste', function () {
            $(".lite-modal #{{ target }}-picklist-picker__container").addClass("app-picklist-picker__container--loading");
            filter{{ target }}PicklistItems(true, $(this).val());
        });
        return false;
    }

function pickList(target) { getting executed on page load. I have to check how to stop this.

Also target is mainly used to format the function name in filter{{ target }}PicklistItems(true);
If we take it as a function parameter, I am not sure this is possible.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Anonymous function is moved into a separate function

LITECommon.Modal.closeAllModals();
}

function setTextareaValue(textareaId) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is updated the target text field with the text from the selected picklist item

approval_reasons = forms.CharField(
widget=forms.Textarea(attrs={"rows": "10"}),
widget=forms.Textarea(attrs={"rows": "10", "style": "margin-top:20px"}),
Copy link
Contributor

@wkeeling wkeeling Dec 2, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm wondering if using a fixed pixel margin-top here is going to work well on other types of devices - e.g. mobile? Is there a govuk specific class that can achieve the same thing?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good point, I will have a quick search and replace accordingly.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@wkeeling there is a govuk class, I have updated to use that class

@saruniitr saruniitr force-pushed the LTD-1345-Select-advice-text-from-picklists branch from 4b790da to e1d190a Compare December 3, 2021 00:13
- Using GOV UK Margin class to have device independent margins
- Using mark_safe may expose us to cross-site scripting vulnerabilities
- Replace anonymous function with a named function
@saruniitr saruniitr force-pushed the LTD-1345-Select-advice-text-from-picklists branch from e1d190a to f64661b Compare December 3, 2021 00:33
label="What are your reasons for approving?",
help_text='<a class="govuk-link govuk-link--no-visited-state" href="#" target="approval_reasons">Choose an approval reason from the template list</a>',
Copy link
Contributor

@alixedi alixedi Dec 3, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we not subclass CharField so that we don't have to put the help_text & widget incantation every time for picklist inputs?

Something like -

class PicklistCharField(forms.CharField):

    def get_help_link(self, target, help_text):
        return f'<a class="govuk-link govuk-link--no-visited-state" href="#" target="{target}">{help_text}</a>'

    def __init__(self, target, help_text, *args, **kwargs):
        help_link = self.get_help_link(target, help_text)
        super.__init__(*args, **kwargs, help_text=help_link, widget=forms.TextArea...)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

<script nonce="{{ request.csp_nonce }}">
$("a[target={{ target }}]").on("click", {"picklist_name": `{{ picklist_name }}`, "target": `{{ target }}`}, showPickListItems);

function showPickListItems(event) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reason I asked you to put this in a separate function was so that this bit is not repeated 3 times on the rendered page.

ATM, whenever you do -

{% include "advice/picklist.html" with target="approval_reasons" picklist_type="standard_advice" picklist_name="Standard Advice" %}

It copies over everything in this template (including this function) into the rendered template. This should be fixable by moving the function to another template and including it once.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moving this function to a template is not bringing up the modal when the link is clicked.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

repetition is avoided now

filter{{ target }}PicklistItems(true);
$(".lite-modal .app-picklist__search-input").on('input propertychange paste', function () {
$(`.lite-modal #${event.data.target}-picklist-picker__container`).addClass("app-picklist-picker__container--loading");
filter{{ target }}PicklistItems(true, $(this).val());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we not replace this with -

def FilterPicklistItems(true, $(this).val(), target)

The fact that we are choosing to template a function where can just pass arguments doesn't make sense.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

moved this into a template and only included once now so no repetition and not templating the function.

@saruniitr saruniitr force-pushed the LTD-1345-Select-advice-text-from-picklists branch 3 times, most recently from 03434b9 to 3990c58 Compare December 5, 2021 00:49
Copy link
Contributor

@alixedi alixedi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One comment that I will leave with you.

A reminder that we will need to add browser tests for this later. Because of all the js.

picklist_tags = f'picklist_type="{picklist_attrs.get("type")}" picklist_name="{picklist_attrs.get("name")}" target="{picklist_attrs.get("target")}"'
return f'<a class="govuk-link govuk-link--no-visited-state" href="#" {picklist_tags}>{help_text}</a>'

def __init__(self, picklist_attrs, label, help_text, required=True, min_rows=10, **kwargs):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can remove required and min_rows from here, no? Because you are passing through kwargs to super and these would be included without having to put them explicitly in the parameters.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, I will remove them.

Since the same pattern is being repeated multiple times a custom field
is created for fields that can be selected from picklists. It also
accepts certain attributes which are used in JS to retrieve
relevant picklists from the API.

The javascript also moved to a template to avoid repeating it for
each field, functions are parameterized so they work for all
picklist target and types.

The picklist picker dialog html also is made generic and is not
repeated for each picklist field.
@saruniitr saruniitr force-pushed the LTD-1345-Select-advice-text-from-picklists branch from 3990c58 to a67bb95 Compare December 6, 2021 10:58
@saruniitr saruniitr merged commit 56df90d into master Dec 6, 2021
@saruniitr saruniitr deleted the LTD-1345-Select-advice-text-from-picklists branch December 6, 2021 11:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants