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

[feat]: add upload widget into the search form tab #730

Closed
wants to merge 2 commits into from

Conversation

meenal06
Copy link
Contributor

@meenal06 meenal06 commented Apr 8, 2021

Which problem is this PR solving?

Short description of the changes

  • Combined upload json and search form page into a single tab

Screenshot

Screenshot 2021-04-09 at 1 18 49 AM

Signed-off-by: Meenal Trivedi <meenaltrivedi6102@gmail.com>
Signed-off-by: Meenal Trivedi <meenaltrivedi6102@gmail.com>
@meenal06
Copy link
Contributor Author

meenal06 commented Apr 8, 2021

Hi @rubenvp8510 @jpkrohling , please review and see if it suits the use-case 😄

@codecov
Copy link

codecov bot commented Apr 8, 2021

Codecov Report

Merging #730 (6824600) into master (b5dd6f9) will increase coverage by 0.03%.
The diff coverage is n/a.

❗ Current head 6824600 differs from pull request most recent head 124037a. Consider uploading reports for the commit 124037a to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master     #730      +/-   ##
==========================================
+ Coverage   94.39%   94.42%   +0.03%     
==========================================
  Files         230      230              
  Lines        5959     5958       -1     
  Branches     1448     1448              
==========================================
+ Hits         5625     5626       +1     
+ Misses        300      298       -2     
  Partials       34       34              
Impacted Files Coverage Δ
...r-ui/src/components/SearchTracePage/FileLoader.tsx 100.00% <ø> (ø)
.../jaeger-ui/src/components/SearchTracePage/index.js 86.66% <ø> (ø)
...nents/TracePage/TraceTimelineViewer/SpanBarRow.tsx 79.31% <0.00%> (+6.89%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b5dd6f9...124037a. Read the comment docs.

@yurishkuro
Copy link
Member

I am not sure this is an improvement. Uploading a file to the UI is not the primary use case, and we're sacrificing a bunch of primary top-level real estate to it.

I think the main issue raised in #683 has been fixed, but changing upload from a separate tab to something else is not a priority imo.

@meenal06
Copy link
Contributor Author

meenal06 commented Apr 8, 2021

I am not sure this is an improvement. Uploading a file to the UI is not the primary use case, and we're sacrificing a bunch of primary top-level real estate to it.

I think the main issue raised in #683 has been fixed, but changing upload from a separate tab to something else is not a priority imo.

So is this enhancement not needed? Or do we need to replace the click to upload with the drag-drop region as originally requested 🤔

@yurishkuro
Copy link
Member

I am curious to see what others think. The second tab never bothered me, and #683 was just one person's opinion (plus I think it was more on the side of compacting the search form, which was done, the upload change was mixed in but really is a separate ask).

@jpkrohling
Copy link
Contributor

Uploading a file to the UI is not the primary use case, and we're sacrificing a bunch of primary top-level real estate to it.

Agree with this.

@rubenvp8510
Copy link
Collaborator

I am not sure this is an improvement. Uploading a file to the UI is not the primary use case, and we're sacrificing a bunch of primary top-level real estate to it.

I agree, this makes sense, +1

I think the main issue raised in #683 has been fixed, but changing upload from a separate tab to something else is not a priority imo.

In #683 search form compaction is an improvement, but I think we leave the upload in a separate tab, is something that I don't think is a big deal.

@meenal06
Copy link
Contributor Author

meenal06 commented Apr 9, 2021

So should I close this PR? Also, can this be added as an outreachy contribution or not 😕 ?

@yurishkuro
Copy link
Member

Yes, let's close the PR.

@jpkrohling might be able to answer re outreachy qualification (I'm +1 to count it in).

@meenal06 meenal06 closed this Apr 9, 2021
@meenal06 meenal06 deleted the upload branch April 9, 2021 21:23
@jpkrohling
Copy link
Contributor

Sorry for missing this: of course it counts! The goal is not to get the most PRs merged but contribute to the project. Sometimes, it's getting a PR merged, other times it's bringing clarity in some way, like what happened here.

@meenal06
Copy link
Contributor Author

Thanks @jpkrohling !

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.

Improve Search box UI and UX
4 participants