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

Change Download JSON button to be a normal link with href. #3040

Merged
merged 3 commits into from
Mar 25, 2020

Conversation

anuraaga
Copy link
Contributor

@anuraaga anuraaga commented Mar 23, 2020

No need to create a magic anchor to do the download instead of making the original button an anchor.

Fixes #3039

@tacigar
Copy link
Member

tacigar commented Mar 23, 2020

hmm...
I cannot download JSON in chrome (but I can in firefox)...

@anuraaga
Copy link
Contributor Author

Ah I only checked on Firefox, let me check Chrome too

@anuraaga
Copy link
Contributor Author

Just tried on Chrome and it worked ok - I do need to fix it to work on dev server using https://create-react-app.dev/docs/proxying-api-requests-in-development/#configuring-the-proxy-manually it only really works on normal server. Just to check, did you try using :9411 or :3000?

@tacigar
Copy link
Member

tacigar commented Mar 23, 2020

I tried using :3000.
:9411 works ok.
The same problem seems to have existed before...

@anuraaga
Copy link
Contributor Author

Thanks for confirming, yeah it broke in the dev server with the react-scripts migrations so will fix!

@anuraaga
Copy link
Contributor Author

anuraaga commented Mar 24, 2020

Fixed the proxy - went ahead and restored our API_BASE variable at the same time since it's easy

@jorgheymans
Copy link
Contributor

jorgheymans commented Mar 24, 2020

maybe this is a good moment to have a deep and meaningful discussion about the placement of the button as well :-P

image

I'm thinking that it could make sense to have the download button next to the upload button instead, and make it an icon only (with tooltip) so they're nicely uniform ? @tacigar also

@anuraaga
Copy link
Contributor Author

I've been thinking the same thing - moving the button up seems good to me.

@anuraaga
Copy link
Contributor Author

One point we'll need to think of is the other buttons. If we move download, we'd presumably move view logs and archive trace too. I'd say the icon for view logs is pretty bad - I desperately searched for something not terrible on font awesome. So icon-only might not be so nice. If we can get an icon designed that looks clearer that'd be very cool :D

@jorgheymans
Copy link
Contributor

This may turn into a slightly bigger UI concept refactoring then, separate issue ? Thing is, there are 'global' widgets like the search trace and upload trace, then you have the trace 'local' stuff like download, view logs and archive. Is it ok mixing these into one "toolbar"?

@anuraaga
Copy link
Contributor Author

Yup true that this would make the global toolbar dynamic - lots of code change as well as a big design change, so good to think about it separately.

@codefromthecrypt
Copy link
Member

as we have some glitches in our last docker images, plus we've that favorite trace thing in master.. if this is good and mergable, let's do it and cut a minor?

@anuraaga anuraaga merged commit 3f213e4 into openzipkin:master Mar 25, 2020
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

Successfully merging this pull request may close these issues.

lens: hovering over [download json] button could show REST url to fetch trace
4 participants