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: Fix dropdown option duplications #10706

Merged
merged 4 commits into from
Jul 27, 2021

Conversation

kaxcode
Copy link
Contributor

@kaxcode kaxcode commented Jul 27, 2021

🐛 Issue: New intentions form - Source Namespace and Destination Namespace drop-downs have options duplicated with onChange action. Issue reported in #10686.

🩹 Solution: Update onChange action to mutate to the selected option. This is following the usage documentation for ember-power-select-with-create package.

Screenshots:

  • Before
    intention_dropdown_before

  • After
    intention_dropdown_after

Tests: ❌

  • Backlog

  • Changelog

@kaxcode kaxcode added theme/ui Anything related to the UI backport/1.10 labels Jul 27, 2021
@kaxcode kaxcode requested review from gsusmi and a user July 27, 2021 20:38
@vercel vercel bot temporarily deployed to Preview – consul-ui-staging July 27, 2021 20:41 Inactive
@vercel vercel bot temporarily deployed to Preview – consul July 27, 2021 20:41 Inactive
Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

Awesome work! Since you're no longer using a named action here, can you drop the action helper and use fn instead?

Copy link

@gsusmi gsusmi left a comment

Choose a reason for hiding this comment

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

LGTM! Agree with note about using fn. Ty!

Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

Sorry just one last point of feedback. SourceNS and DestinationNS are ambiguous without a @ or this. in front of them. Going forward this will be required in Ember. Not sure if you all are worried about this yet. If not, disregard!

@kaxcode
Copy link
Contributor Author

kaxcode commented Jul 27, 2021

Sorry just one last point of feedback. SourceNS and DestinationNS are ambiguous without a @ or this. in front of them. Going forward this will be required in Ember. Not sure if you all are worried about this yet. If not, disregard!

Yes, let me update that as well.

@kaxcode kaxcode merged commit eb5512f into main Jul 27, 2021
@kaxcode kaxcode deleted the ui/bug/new-intentions-dropdown-option-duplication branch July 27, 2021 21:34
@hc-github-team-consul-core
Copy link
Contributor

🍒 If backport labels were added before merging, cherry-picking will start automatically.

To retroactively trigger a backport after merging, add backport labels and re-run https://circleci.com/gh/hashicorp/consul/417284.

@hc-github-team-consul-core
Copy link
Contributor

🍒✅ Cherry pick of commit eb5512f onto release/1.10.x succeeded!

@hc-github-team-consul-core
Copy link
Contributor

🍒 If backport labels were added before merging, cherry-picking will start automatically.

To retroactively trigger a backport after merging, add backport labels and re-run https://circleci.com/gh/hashicorp/consul/417943.

@hc-github-team-consul-core
Copy link
Contributor

🍒❌ Cherry pick of commit eb5512f onto release/1.10.x failed! Build Log

@hc-github-team-consul-core
Copy link
Contributor

🍒✅ Cherry pick of commit eb5512f onto release/1.9.x succeeded!

@hc-github-team-consul-core
Copy link
Contributor

🍒 If backport labels were added before merging, cherry-picking will start automatically.

To retroactively trigger a backport after merging, add backport labels and re-run https://circleci.com/gh/hashicorp/consul/417971.

@hc-github-team-consul-core
Copy link
Contributor

🍒❌ Cherry pick of commit eb5512f onto release/1.10.x failed! Build Log

@hc-github-team-consul-core
Copy link
Contributor

🍒❌ Cherry pick of commit eb5512f onto release/1.9.x failed! Build Log

@hc-github-team-consul-core
Copy link
Contributor

🍒 If backport labels were added before merging, cherry-picking will start automatically.

To retroactively trigger a backport after merging, add backport labels and re-run https://circleci.com/gh/hashicorp/consul/417972.

@hc-github-team-consul-core
Copy link
Contributor

🍒❌ Cherry pick of commit eb5512f onto release/1.10.x failed! Build Log

@hc-github-team-consul-core
Copy link
Contributor

🍒❌ Cherry pick of commit eb5512f onto release/1.9.x failed! Build Log

@hc-github-team-consul-core
Copy link
Contributor

🍒 If backport labels were added before merging, cherry-picking will start automatically.

To retroactively trigger a backport after merging, add backport labels and re-run https://circleci.com/gh/hashicorp/consul/417988.

@hc-github-team-consul-core
Copy link
Contributor

🍒❌ Cherry pick of commit eb5512f onto release/1.9.x failed! Build Log

johncowen pushed a commit that referenced this pull request Sep 21, 2021
johncowen added a commit that referenced this pull request Sep 22, 2021
)

This PR supersedes #10706 and fixes #10686 whilst making sure that saving intentions continues to work.

The original fix in #10706 ignored the change action configured for the change event on the menus, meaning that the selected source/destination namespace could not be set by the user when editing/creating intentions. This, coupled with the fact that using the later intention exact endpoint for API requests endpoint means that you could not use wildcard namespaces for saving intentions.

All in all this meant that intentions could no longer be saved using the UI (whilst using ENT)

This PR reverts #10706 to fix the intention saving issue, and adds a fix for the original visual issue of nspaces doubling up in the menu once clicked. This meant repeating the existing functionality for nspaces aswell as services. It did seem strange to me that the original issue was only apparent for the nspace menus and not the service menus which should all function exactly the same way.

There is potentially more to come here partly related to what the exact functionality should be, but I'm working with other folks to figure out what the best way forwards is longer term. In the meantime this brings us back to the original functionality with the visual issue fixed.

Squashed commits:

* Revert "ui: Fix dropdown option duplications (#10706)"

This reverts commit eb5512f.

* ui: Ensure additional nspaces are added to the unique list of nspaces

* Add some acceptance tests
hc-github-team-consul-core pushed a commit that referenced this pull request Sep 22, 2021
)

This PR supersedes #10706 and fixes #10686 whilst making sure that saving intentions continues to work.

The original fix in #10706 ignored the change action configured for the change event on the menus, meaning that the selected source/destination namespace could not be set by the user when editing/creating intentions. This, coupled with the fact that using the later intention exact endpoint for API requests endpoint means that you could not use wildcard namespaces for saving intentions.

All in all this meant that intentions could no longer be saved using the UI (whilst using ENT)

This PR reverts #10706 to fix the intention saving issue, and adds a fix for the original visual issue of nspaces doubling up in the menu once clicked. This meant repeating the existing functionality for nspaces aswell as services. It did seem strange to me that the original issue was only apparent for the nspace menus and not the service menus which should all function exactly the same way.

There is potentially more to come here partly related to what the exact functionality should be, but I'm working with other folks to figure out what the best way forwards is longer term. In the meantime this brings us back to the original functionality with the visual issue fixed.

Squashed commits:

* Revert "ui: Fix dropdown option duplications (#10706)"

This reverts commit eb5512f.

* ui: Ensure additional nspaces are added to the unique list of nspaces

* Add some acceptance tests
hc-github-team-consul-core pushed a commit that referenced this pull request Sep 22, 2021
)

This PR supersedes #10706 and fixes #10686 whilst making sure that saving intentions continues to work.

The original fix in #10706 ignored the change action configured for the change event on the menus, meaning that the selected source/destination namespace could not be set by the user when editing/creating intentions. This, coupled with the fact that using the later intention exact endpoint for API requests endpoint means that you could not use wildcard namespaces for saving intentions.

All in all this meant that intentions could no longer be saved using the UI (whilst using ENT)

This PR reverts #10706 to fix the intention saving issue, and adds a fix for the original visual issue of nspaces doubling up in the menu once clicked. This meant repeating the existing functionality for nspaces aswell as services. It did seem strange to me that the original issue was only apparent for the nspace menus and not the service menus which should all function exactly the same way.

There is potentially more to come here partly related to what the exact functionality should be, but I'm working with other folks to figure out what the best way forwards is longer term. In the meantime this brings us back to the original functionality with the visual issue fixed.

Squashed commits:

* Revert "ui: Fix dropdown option duplications (#10706)"

This reverts commit eb5512f.

* ui: Ensure additional nspaces are added to the unique list of nspaces

* Add some acceptance tests
hc-github-team-consul-core pushed a commit that referenced this pull request Sep 22, 2021
)

This PR supersedes #10706 and fixes #10686 whilst making sure that saving intentions continues to work.

The original fix in #10706 ignored the change action configured for the change event on the menus, meaning that the selected source/destination namespace could not be set by the user when editing/creating intentions. This, coupled with the fact that using the later intention exact endpoint for API requests endpoint means that you could not use wildcard namespaces for saving intentions.

All in all this meant that intentions could no longer be saved using the UI (whilst using ENT)

This PR reverts #10706 to fix the intention saving issue, and adds a fix for the original visual issue of nspaces doubling up in the menu once clicked. This meant repeating the existing functionality for nspaces aswell as services. It did seem strange to me that the original issue was only apparent for the nspace menus and not the service menus which should all function exactly the same way.

There is potentially more to come here partly related to what the exact functionality should be, but I'm working with other folks to figure out what the best way forwards is longer term. In the meantime this brings us back to the original functionality with the visual issue fixed.

Squashed commits:

* Revert "ui: Fix dropdown option duplications (#10706)"

This reverts commit eb5512f.

* ui: Ensure additional nspaces are added to the unique list of nspaces

* Add some acceptance tests
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
theme/ui Anything related to the UI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants