-
Notifications
You must be signed in to change notification settings - Fork 50
fix: Settings page sends user to settings/labels in new iox orgs #6543
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
Conversation
src/shared/containers/SetOrg.tsx
Outdated
| exact | ||
| path={`${orgPath}/${SETTINGS}`} | ||
| component={VariablesIndex} | ||
| component={isNewIOxOrg ? LabelsIndex : VariablesIndex} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of conditionally rendering variables or labels, (which will persist after the conditions that require the conditional are gone) why don't we just make Labels the default page under settings? That way it'll just work, whether the org has variables or not
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could do that - it'll just mean it changes the UI experience for users who aren't new IOx.
@garylfowler @gunnaraasen Does it matter whether the "Labels" page or the "Variables" page renders by default when users visit Settings? If we switch from Variables to "Labels" we won't need a different routing behavior for the "Settings" page depending on whether the user is in a New IOx org or not.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think it matters.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unless they disagree, I'm inclined to leave it as-is for now, just because Labels isn't the first tab. Reordering the tabs out of alpha order seems like a little yak-shave-y for just fixing the default page.
|
@hoorayimhelping Ended up going with your approach, it's simpler. 👍 |
blegesse-w
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
In local testing, if I simulate the conditions of being a new IOx org, clicking the settings link gives me a 404. The reason is that the link is still hardcoded to send the user to variables by default, which won't exist for new IOx orgs.
Solution is to send the user to labels by default if it's a new IOx org.
Checklist
Authors and Reviewer(s), please verify the following: