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: ugly urls #7777

Closed
BramGruneir opened this issue Jul 12, 2016 · 13 comments
Closed

ui: ugly urls #7777

BramGruneir opened this issue Jul 12, 2016 · 13 comments
Assignees
Milestone

Comments

@BramGruneir
Copy link
Member

When checking node status, I get redirected to this url
http://104.196.56.219:8080/#/nodes/overview?_k=7kazps

The url should just be, and all options should always be human readble.
http://104.196.56.219:8080/#/nodes

This is true for all urls, there's always a k option added that shouldn't be there.
Another example the cluster page shouldn't be:
http://104.196.56.219:8080/#/cluster?_k=k1odh2
It should be
http://104.196.56.219:8080/#/cluster

@tamird
Copy link
Contributor

tamird commented Jul 12, 2016

OK but who cares?

On Tue, Jul 12, 2016 at 12:00 PM, Bram Gruneir notifications@github.com
wrote:

When checking node status, I get redirected to this url
http://104.196.56.219:8080/#/nodes/overview?_k=7kazps

The url should just be, and all options should always be human readble.
http://104.196.56.219:8080/#/nodes

This is true for all urls, there's always a k option added that shouldn't
be there.
Another example the cluster page shouldn't be:
http://104.196.56.219:8080/#/cluster?_k=k1odh2
It should be
http://104.196.56.219:8080/#/cluster


You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
#7777, or mute the thread
https://github.com/notifications/unsubscribe/ABdsPFKf6-iylFCpufUjCxaNQcSZcwl4ks5qU7o5gaJpZM4JKjF0
.

@BramGruneir
Copy link
Member Author

I do.
And I'm going to bet that our designers will as well.

If there was a clear need for this value it would be one thing, but it's an
option that can be turned off and should be.

On Tue, Jul 12, 2016 at 12:12 PM, Tamir Duberstein <notifications@github.com

wrote:

OK but who cares?

On Tue, Jul 12, 2016 at 12:00 PM, Bram Gruneir notifications@github.com
wrote:

When checking node status, I get redirected to this url
http://104.196.56.219:8080/#/nodes/overview?_k=7kazps

The url should just be, and all options should always be human readble.
http://104.196.56.219:8080/#/nodes

This is true for all urls, there's always a k option added that shouldn't
be there.
Another example the cluster page shouldn't be:
http://104.196.56.219:8080/#/cluster?_k=k1odh2
It should be
http://104.196.56.219:8080/#/cluster


You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
#7777, or mute the
thread
<
https://github.com/notifications/unsubscribe/ABdsPFKf6-iylFCpufUjCxaNQcSZcwl4ks5qU7o5gaJpZM4JKjF0

.


You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#7777 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/ABihubECutIgbx-fj1-4C0N4ZKiLzm82ks5qU7zagaJpZM4JKjF0
.

@tamird
Copy link
Contributor

tamird commented Jul 12, 2016

If you know how to turn it off, care to mention that in the issue
description?

On Tue, Jul 12, 2016 at 12:21 PM, Bram Gruneir notifications@github.com
wrote:

I do.
And I'm going to bet that our designers will as well.

If there was a clear need for this value it would be one thing, but it's an
option that can be turned off and should be.

On Tue, Jul 12, 2016 at 12:12 PM, Tamir Duberstein <
notifications@github.com

wrote:

OK but who cares?

On Tue, Jul 12, 2016 at 12:00 PM, Bram Gruneir <notifications@github.com

wrote:

When checking node status, I get redirected to this url
http://104.196.56.219:8080/#/nodes/overview?_k=7kazps

The url should just be, and all options should always be human readble.
http://104.196.56.219:8080/#/nodes

This is true for all urls, there's always a k option added that
shouldn't
be there.
Another example the cluster page shouldn't be:
http://104.196.56.219:8080/#/cluster?_k=k1odh2
It should be
http://104.196.56.219:8080/#/cluster


You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
#7777, or mute the
thread
<

https://github.com/notifications/unsubscribe/ABdsPFKf6-iylFCpufUjCxaNQcSZcwl4ks5qU7o5gaJpZM4JKjF0

.


You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<
#7777 (comment)
,
or mute the thread
<
https://github.com/notifications/unsubscribe/ABihubECutIgbx-fj1-4C0N4ZKiLzm82ks5qU7zagaJpZM4JKjF0

.


You are receiving this because you commented.
Reply to this email directly, view it on GitHub
#7777 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/ABdsPG84wDAjQxYSf9YlmFny-CxDHwYZks5qU78kgaJpZM4JKjF0
.

@maxlang maxlang self-assigned this Jul 12, 2016
@maxlang
Copy link
Contributor

maxlang commented Jul 12, 2016

@BramGruneir
Copy link
Member Author

@tamird I didn't know that at the time. Found it after some discussion with @maxlang and I couldn't update the issue due to github errors.

@mrtracy
Copy link
Contributor

mrtracy commented Jul 12, 2016

I agree that this is a non-issue, but if it's trivial to take out (and doesn't have consequences) we can do it.

@BramGruneir
Copy link
Member Author

Really, we should be using browserHistory instead of hashHistory.

https://github.com/reactjs/react-router/blob/6117def486a813221d0d254b0b4920afbbc51d48/docs/guides/Histories.md#browserhistory

The only reason not to is because of older browser compatibility, which isn't an issue for us.

@tbg
Copy link
Member

tbg commented Jul 13, 2016

Isn't older browser compatibility an issue for us?

On Wed, Jul 13, 2016 at 11:06 AM Bram Gruneir notifications@github.com
wrote:

Really, we should be using browserHistory instead of hashHistory.

https://github.com/reactjs/react-router/blob/6117def486a813221d0d254b0b4920afbbc51d48/docs/guides/Histories.md#browserhistory

The only reason not to is because of older browser compatibility, which
isn't an issue for us.


You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
#7777 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/AE135Oj_Bg5oOs8zbM_osF9ijOShajCBks5qVP79gaJpZM4JKjF0
.

-- Tobias

@BramGruneir
Copy link
Member Author

Why would it be?
Are we really worried about IE8? Or very old versions of chrome or firefox?

I think it's fair to assume that those that are using the ui have fairly up to date browsers.

And even if they don't, it only means a full reload instead of a partial page reload, so it will still be functional.

@BramGruneir
Copy link
Member Author

Also, by going with browserHistory, we remove the hash from the urls.

@tamird tamird self-assigned this Jul 13, 2016
@tbg
Copy link
Member

tbg commented Jul 13, 2016

I'm not familiar with the tradeoffs, just asking. I'm sure we need some
compatibility with older browsers. I don't think we've worried about it
much, though.

On Wed, Jul 13, 2016 at 11:12 AM Bram Gruneir notifications@github.com
wrote:

Also, by going with browserHistory, we remove the hash from the urls.


You are receiving this because you commented.

Reply to this email directly, view it on GitHub
#7777 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/AE135IFGk23extzLBuyLouNnP1yzak3uks5qVQBdgaJpZM4JKjF0
.

-- Tobias

@maxlang
Copy link
Contributor

maxlang commented Aug 1, 2016

This should be reopened since the PR was reverted.

@maxlang maxlang reopened this Aug 1, 2016
@maxlang maxlang removed their assignment Aug 9, 2016
@tamird tamird modified the milestone: Later Aug 29, 2016
@mrtracy
Copy link
Contributor

mrtracy commented Jan 26, 2017

I'm just going to close this as WontFix. We cannot support BrowserHistory without server-side support, and react-router's HashHistory appears to think the extra bit in the query string is important for reliable back-button navigation.

@mrtracy mrtracy closed this as completed Jan 26, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants