Skip to content

Improve Presto Query Page UI#13158

Merged
arhimondr merged 2 commits intoprestodb:masterfrom
nausicaasnow:master
Aug 6, 2019
Merged

Improve Presto Query Page UI#13158
arhimondr merged 2 commits intoprestodb:masterfrom
nausicaasnow:master

Conversation

@nausicaasnow
Copy link
Copy Markdown
Contributor

@nausicaasnow nausicaasnow commented Jul 30, 2019

Distribute the tasks into every stage and only display when expanding stage summary, which is to avoid tasks flushing, the auto-fresh behaviors of tasks are now consistent with stage's.

The 'None' is removed from task filter, the default value now is 'ALL'.

image

image

== RELEASE NOTES ==

General Changes
* Display tasks per stage on the query page

Copy link
Copy Markdown
Member

@arhimondr arhimondr left a comment

Choose a reason for hiding this comment

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

@nausicaasnow Thanks for working on this!

Generally looks good. Few comments:

  1. Could you please tweak the layout a little bit, so the head of the tasks table and the data are aligned
    62167261-ac4dbf80-b2d7-11e9-9cb6-a0865c5062ee

  2. Could you please do the rebase git fetch origin; git rebase -i origin/master; and squash the merge commit? So the change has only a single commit?

  3. You need to rebuild the UI and commit the generated JS, otherwise the build won't pass.

  4. nit and optional: Usually we are trying to minimize the amount of changes needed. The reformatting the table header makes code better but it isn't a part of the solution to the problem, thus i would suggest to put it into a separate commit.

@arhimondr
Copy link
Copy Markdown
Member

arhimondr commented Jul 31, 2019

Let's give a chance for @ggreg to have a look

@rongrong
Copy link
Copy Markdown
Contributor

rongrong commented Aug 2, 2019

Second @arhimondr's comments. Otherwise the change looks good to me. Thanks a lot for working on this!

Distribute the tasks into every stage and only display when expanding
stage summary, which is to avoid tasks flushing, the auto-fresh behaviors
of tasks are now consistent with stage's.

The 'None' is removed from task filter, the default value now is 'ALL'.
@arhimondr
Copy link
Copy Markdown
Member

Updated:

Selection_122

Copy link
Copy Markdown
Contributor

@rongrong rongrong left a comment

Choose a reason for hiding this comment

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

Looks good! Since there's a reformatting commit, is this the only reformat needed?

@arhimondr
Copy link
Copy Markdown
Member

Since there's a reformatting commit, is this the only reformat needed?

Not sure. I was just trying to preserve the original code =)

@rongrong
Copy link
Copy Markdown
Contributor

rongrong commented Aug 6, 2019

Not sure. I was just trying to preserve the original code =)

Well my review for the initial code would also be "remove reformatting since it's not related or maybe try to do a better job". But I won't be too picky since it's strictly better than before.

@arhimondr
Copy link
Copy Markdown
Member

The product test failure is unrelated

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.

4 participants