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

Set State Modal #465

Merged
merged 16 commits into from
Dec 5, 2020
Merged

Set State Modal #465

merged 16 commits into from
Dec 5, 2020

Conversation

ThatGalNatalie
Copy link
Contributor

@ThatGalNatalie ThatGalNatalie commented Nov 25, 2020

PR Checklist:

  • add a short description of what's changed to the top of the CHANGELOG.md
  • add/update tests (or don't, for reasons explained below)

Describe this PR

Changes the design of the Set State modal to match the design of the other forms

@ThatGalNatalie ThatGalNatalie linked an issue Nov 25, 2020 that may be closed by this pull request
3 tasks
@znicholasbrown
Copy link
Contributor

This is looking really nice @ThatGalNatalie, 2 initial requests before code review:

  1. The autocomplete for the optional state change reason is trying to autocomplete with a geographic state - I think we can use autocomplete="off" so it doesn't do that
  2. Could we add a loading state to the Confirm button so that it can't be clicked multiple times?

@ThatGalNatalie ThatGalNatalie marked this pull request as ready for review December 1, 2020 21:08
src/components/SetStateDialog.vue Outdated Show resolved Hide resolved
src/pages/FlowRun/FlowRun.vue Outdated Show resolved Hide resolved
src/components/SetStateDialog.vue Show resolved Hide resolved
<v-card flat :loading="markAsLoading">
<div style="padding: 20px;">
<v-card-title class="headline word-break-normal mb-3" primary-title>
Change the state of {{ taskRun ? taskRun.task.name : flowRun.name }}
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's differentiate the task run/flow run name here. Also I think this will read:

Change the state of

for task runs without names, so we should add a check for that and use the task name with something like "run of <>" if it's not available.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

okay so something like this?

 Change the run of
            {{
              taskRun
                ? taskRun.task.name
                : taskRun && !taskRun.task.name
                ? taskRun.name
                : flowRun.name
            }}

Copy link
Contributor

@znicholasbrown znicholasbrown Dec 2, 2020

Choose a reason for hiding this comment

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

Hm I think this:

<span v-if="taskRun && taskRun.name">
 Change the state of
            {{ taskRun.name }}
</span>
<span v-else-if="taskRun">
Change the state of this run of
{{ taskRun.task.name }}
</span>
<span v-else>{{ flowRun.name }}</span>

with added checks if there are points where those might loading

@ThatGalNatalie
Copy link
Contributor Author

Okay I think it's good to go now unless there was something else? @znicholasbrown

Copy link
Member

@zhen0 zhen0 left a comment

Choose a reason for hiding this comment

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

This looks great - really like the new design @ThatGalNatalie One suggestion (possibly cutting some of the tooltip wording) and one requested change (the css that affects other components) from me.

Comment on lines 133 to 144
Please be aware that clicking on confirm will set the state of
your
{{ dialogType }}
{{
taskRun && taskRun.name
? taskRun.name
: taskRun
? taskRun.task.name
: flowRun.name
}}
to
<span class="font-weight-black pb-8"> {{ selectedState }}.</span>
Copy link
Member

@zhen0 zhen0 Dec 4, 2020

Choose a reason for hiding this comment

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

Suggestion - I actually think we could take all of this out. I know it was in the old version and the tooltip is a great addition for the necessary warnings about child and downstream tasks but I'm not sure this section/warning adds much and possibly detracts from the more important warnings.

margin-top: -20px;
}
/* stylelint-disable */
.v-btn__loader {
Copy link
Member

@zhen0 zhen0 Dec 4, 2020

Choose a reason for hiding this comment

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

Editing this vuetify class risks affecting every button loader in the app (same for the theme light subheader above). You can see the affect of this in the flow tile if you delete a label from this branch - the loader is not visible (but is in dev). Elsewhere we get around this issue by placing the Vuetify class inside of a parent class - Happy to pair on this if you're not sure what I mean putting it in inside a parent class (or there's examples in other places e.g. the flow summary tile.)

Copy link
Member

@zhen0 zhen0 left a comment

Choose a reason for hiding this comment

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

LGTM @ThatGalNatalie - I'll let @znicholasbrown give final approval.

Copy link
Contributor

@znicholasbrown znicholasbrown left a comment

Choose a reason for hiding this comment

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

LGTM @ThatGalNatalie 👍

@znicholasbrown znicholasbrown merged commit 959d317 into master Dec 5, 2020
@znicholasbrown znicholasbrown deleted the set-state-modal branch December 5, 2020 01:10
@zhen0 zhen0 mentioned this pull request Dec 7, 2020
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.

Improve the design of the Set State modal
3 participants