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.

Sign up for GitHub

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

Jump to bottom

UI cleanup follow up from #3245. #3251

Merged
merged 3 commits into from
Jul 10, 2017
Merged

UI cleanup follow up from #3245. #3251

merged 3 commits into from
Jul 10, 2017

Conversation

slackpad
Copy link
Contributor

https://github.com/hashicorp/consul/pull/3245/files#r126311704

While I was in here I also made the UI endpoint return a 404 for a missing node, rather than a 200 with no body.

@slackpad slackpad requested a review from pearkes July 10, 2017 16:17
pearkes
pearkes approved these changes Jul 10, 2017
View reviewed changes
Copy link
Contributor

@pearkes pearkes left a comment

Choose a reason for hiding this comment

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

LGTM!
I believe if it 404s the UI will treat that as a general error which is why it was 200 before. That may be ok but worth trying out the experience to be sure you can nav back or whatever. Browsers may have evolved, but I don't know.

@slackpad
Copy link
Contributor Author

The 200 would pop up the error page:

image

So the 404 is definitely an improvement :-)

@slackpad slackpad merged commit 7200b8c into master Jul 10, 2017
@slackpad slackpad deleted the ui-cleanup-redux branch July 10, 2017 16:40
johncowen pushed a commit that referenced this pull request May 4, 2018
@slackpad
UI cleanup follow up from #3245. (#3251)
949acfb 
* Removes unnecessary set for model component which will be null.

* Returns a 404 for a missing node, not a 200 with an empty response.

* Updates built-in web assets.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pearkes pearkes pearkes approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@slackpad @pearkes