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

Group radio buttons when setting query parameters #312

Merged
merged 2 commits into from
Sep 2, 2021

Conversation

stelar7
Copy link
Contributor

@stelar7 stelar7 commented Sep 2, 2021

Radio buttons would call _.set for each of them, meaning that the value of the parameter would always be the set to the same as the last added. (false in the case of boolean validations)

With this change, the selected value is set instead.

…ed value is added

Radio buttons would call `_.set` for each of them, meaning that the value of the parameter would always be the set to the same as the last added.

With this change, the selected value is set instead.
Copy link
Contributor

@shalvah shalvah left a comment

Choose a reason for hiding this comment

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

I like how clean this code looks, but can you add a comment or two that explains what's going on at a high-level?

Also, if we're undoing something, isn't it possible to avoid it in the first place (ie avoid setting the radio button)?

@shalvah shalvah merged commit 2ff7c29 into knuckleswtf:master Sep 2, 2021
@@ -178,6 +178,21 @@ async function executeTryOut(endpointId, form) {
const query = {};
const queryParameters = form.querySelectorAll('input[data-component=query]');
queryParameters.forEach(el => _.set(query, el.name, el.value));
Copy link
Contributor

@shalvah shalvah Sep 2, 2021

Choose a reason for hiding this comment

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

You know, I think this would have just been simpler:

    queryParameters.forEach(el => {
        if (el.type !== 'radio' || (el.type === 'radio' && el.checked)) {
            _.set(query, el.name, el.value);
        }
    });

I'll change it. Simpler is better. 😅

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants