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

Ui/replication merge cleanup #9188

Conversation

Monkeychip
Copy link
Contributor

@Monkeychip Monkeychip commented Jun 10, 2020

This PR address a couple of minor fixes I noticed after the merge. These fixes are not related to the merge, but just things I came across while double-checking.

Fixes in PR:

  • replace with replicationModeForDisplay
  • fix a refresh issue on the primary_cluster_addr that was due to a spelling error and add in default. Confirmed with design.
  • remove extra div with box class per Chelsea's comment
  • change manage link on known secondaries card to take you to the secondaries manage and not the cluster manage page.
  • fix some overflow scroll issues on the sticky-header class, still working on this as I add in the empty state and do some QA on it.
  • Capture escape and enter keyboard actions. Note: for the modal's (disable and demote) where you have to type the mode (e.g. Performance or Disaster Recovery) the escape is captured only on the modal level. This means that escape only closes the modal if you have clicked on the input. I only handle it on the modal, because if I handle the keypress outside these models, I don't know if the confirmText and confirmationText are the same. It gets super complicated handling the submit action after that. The only way I could see to get it to work (if I was actually able to do it) added way more complexity than it was worth.

Here is a gif of the modal actions. I am not using my mouse in any of these, I'm just using the keyboard. If you wanted to test this yourself, make sure to not only hit enter and escape but other keys and try to use your mouse as well. I've tested this pretty extensively, but more testing wouldn't hurt.

Note: There is an error currently when you demote to a DR secondary. Chelsea is fixing that in another PR.

@Monkeychip Monkeychip added the ui label Jun 10, 2020
primaryClusterAddr: computed('replcationDetails.{primaryClusterAddr}', function() {
return this.replicationDetails.primaryClusterAddr;
primaryClusterAddr: computed('replicationDetails.{primaryClusterAddr}', function() {
return this.replicationDetails.primaryClusterAddr || 'Not defined';
Copy link
Contributor

Choose a reason for hiding this comment

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

i was wondering about whether to display a default value here too, but when i spoke to ivana about it yesterday we decided not the show the primary_cluster_addr if one wasn't explicitly set. this is because the primary_cluster_addr is an optional field. showing Not defined here is a little confusing because it almost sounds like an error -- like the user should have defined one but didn't, when in actuality they don't need to define one. what do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let's chat during sync or after. I also spoke with Ivana and that's where I came up with the text. Probably best to make sure we are all on the same page.

@noelledaley
Copy link
Contributor

noelledaley commented Jun 10, 2020

oh boy, i love a good cleanup PR. thanks so much for doing this!

✅ making the modal submit by pressing 'enter' sounds awesome
❓ i had one, non-blocking question regarding the default value for the primary cluster addr
❓ regarding the known_primary_cluster_addr -- would there ever be an empty state? i've always seen at least 1 value in there, which is the value of the main primary. when would that be missing?

other than that, the code looks good. let me know when you need a final 👁️ !

…e former deals with input on the modal footer, the later get's caught in replication actions.
class="input has-margin-top"
autocomplete="off"
spellcheck="false"
enter=onConfirm
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Handle enter on modals that have inputs (e.g. demote where you have to type in the mode to confirm).

@Monkeychip
Copy link
Contributor Author

❓ regarding the known_primary_cluster_addr -- would there ever be an empty state? I've always seen at least 1 value in there, which is the value of the main primary. when would that be missing?

I get an empty state when I demote a primary. Here is the final empty state design. The wording and designed confirmed from Ivana and support.
image


export default Actions.extend({
layout,
onSubmit() {},
Copy link
Contributor Author

Choose a reason for hiding this comment

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

You'll see this added to each replicationAction component. The sole purpose of this is to handle the escape action. The only two modals that don't have this are the demote and disable modals, as they require you type in a confirmationText. If I try and handle the keypress value at this level, I don't know if they've confirmed the text or not, and thus can't handle it this high up. Instead, the escape action is handled on the ConfrimationModal level for these two components.


export default Actions.extend({
layout,
onSubmit() {},
Copy link
Contributor Author

Choose a reason for hiding this comment

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

You'll see this added to each replicationAction component. The sole purpose of this is to handle the escape action. The only two modals that don't have this are the demote and disable modals, as they require you type in a confirmationText. If I try and handle the keypress value at this level, I don't know if they've confirmed the text or not, and thus can't handle it this high up. Instead, the escape action is handled on the ConfrimationModal level for these two components.

Promote this cluster to a {{replicationDisplayMode}} primary
</p>
</div>
<div onkeyup={{action "onSubmit" "promote" model.replicationAttrs.modeForUrl (hash dr_operation_token=dr_operation_token primary_cluster_addr=primary_cluster_addr force=force)}}>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

wrapping div on all replicationAction components (but disable and demote) so that they trigger the check on escape.

id="primary_cluster_addr"
name="primary_cluster_addr"
value=primary_cluster_addr
enter=(action "onSubmit" "promote" model.replicationAttrs.modeForUrl (hash dr_operation_token=dr_operation_token primary_cluster_addr=primary_cluster_addr force=force))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

allowing the enter button to submit when you are in the input of the primary_cluster_addr

@header="cluster_addr"
@items={{knownPrimaryClusterAddrs}}
/>
{{#if (is-empty knownPrimaryClusterAddrs)}}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Adding empty state. It looks like this:

class="input has-margin-top"
autocomplete="off"
spellcheck="false"
enter=(if (eq confirmationInput confirmText) onConfirm)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

handling enter and escape on the confirmation modal used by the replicationAction disable and demote.

Attempt recovery if replication is in a bad state.
</p>
</div>
<div onkeyup={{action "onSubmit" "recover"}}>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Another wrapping div to handle the escape

<p>
Reindex the local data storage
</p>
<div onkeyup={{action 'onSubmit' 'reindex'}}>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Another wrapping div to handle the escape

Change this secondary's assigned primary cluster
</p>
</div>
<div onkeyup={{action "onSubmit" "update-primary" model.replicationAttrs.modeForUrl (hash token=token primary_api_addr=primary_api_addr ca_path=ca_path ca_file=ca_file)}}>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Another wrapping div to handle the escape

@Monkeychip Monkeychip marked this pull request as ready for review June 11, 2020 17:48
@Monkeychip
Copy link
Contributor Author

We removed the changes that were not the modal enter/escape changes and made a new PR. The modal enter/escape functionality is going to be a separate ticket/project that needs more consideration and research regarding accessibility.

@Monkeychip Monkeychip closed this Jun 12, 2020
@Monkeychip Monkeychip deleted the ui/replication-merge-cleanup branch June 7, 2022 20:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants