UI: Region Switcher #4572
UI: Region Switcher #4572
Conversation
… when the gutter is collapsed
This is incredibly tricky with query params, since there is a bundle of timing issues, lifecycle issues, missing features, and all around gotchas with query params. This solution has no observers and no instances of the system service being set from the jobs controller. The upside to this is no observers, much easier to follow logic, no more dependent key chain reactions.
Default params don't make it into arguments which were being splatted into the call to super.
The default region shouldn't be used as a qp since no qp means the same thing.
This protects against volatility in the server and, more commonly, ACLs returning forbidden responses.
The UI will no longer try to redirect to the appropriate namespace or region if one is found in localStorage. Instead, it will assume that the lack of query param means the default namespaces or region is desired.
Without this, the data (query params) get json stringified
|
Looks great visually. The way you've handled in the tablet/mobile menu is 👌too. |
DingoEatingFuzz
force-pushed
the
f-ui-region-switcher
branch
from
74416cf
to
95e8259
Compare
ui/app/routes/application.js
Outdated
| }, | ||
|
|
||
| // setupController doesn't refire when the model hook refires as part of | ||
| // a query param change |
There was a problem hiding this comment.
😳
Is that because the model hook is missing from this route? My understanding of the rule was that setupController fires on entering the route, and when model returns a truthy value.
If you had a trivial model hook, maybe that would cause setupController to fire?
model(params) {
return params.region;
},There was a problem hiding this comment.
Innnnterresting. I'll give it a try.
| import { namespace } from '../adapters/application'; | ||
|
|
||
| // When the request isn't ok (e.g., forbidden) handle gracefully | ||
| const jsonWithDefault = defaultResponse => res => | ||
| res.ok ? res.json() : copy(defaultResponse, true); |
| return null; | ||
| }, | ||
| set(key, value) { | ||
| if (value == null) { |
There was a problem hiding this comment.
Is this loose equality intentional?
There was a problem hiding this comment.
Yeah, to be truthy for undefined as well.
| @@ -2,6 +2,7 @@ import config from 'nomad-ui/config/environment'; | |||
|
|
|||
| const withNamespaces = getConfigValue('mirageWithNamespaces', false); | |||
| const withTokens = getConfigValue('mirageWithTokens', true); | |||
| const withRegions = getConfigValue('mirageWithRegions', false); | |||
There was a problem hiding this comment.
Might want to add this setting to config/environment.js alongside the other Mirage options?
There was a problem hiding this comment.
Heh, definitely. I added it locally, but I so rarely check in changes to the environment file that I forgot I had changes worth committing. Good spot.
| }); | ||
|
|
||
| function forwardNamespace(source, destination) { | ||
| return observer(source, `${source}.id`, function() { |
There was a problem hiding this comment.
It didn't make it all the way through the branch, but I just wanted to throw out a 👍 for the afterSetup route hook approach you originally took to replace this observer:
👍
There was a problem hiding this comment.
Which one was that? I have already suppressed all memories of this code 😅
This is a more idiomatic way to handle the QP resetting in the application controller.
|
I'm going to lock this pull request because it has been closed for 120 days ⏳. This helps our maintainers find and focus on the active contributions. |
The UI now supports multi-region Nomad clusters 🎉
It works by providing the
region=query param to every API request and on every page URL when the active region isn't the region of the server the UI is served from.Reviewer notes: This has been an uphill battle against ember's handling of query params and ember data's handling of emptying the store.