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

Migrate Admin pages to React #3568

Merged
merged 9 commits into from
Mar 27, 2019
Merged

Migrate Admin pages to React #3568

merged 9 commits into from
Mar 27, 2019

Conversation

kravets-levko
Copy link
Collaborator

@kravets-levko kravets-levko commented Mar 11, 2019

What type of PR is this? (check all applicable)

  • Refactor
  • Feature

Description

  • Migrate System Status page
  • Refine Celery Status page
  • Migrate Outdated Queries page

Mobile & Desktop Screenshots/Recordings (if there are UI changes)

image

image

image

@kravets-levko kravets-levko added Frontend Frontend: React Frontend codebase migration to React labels Mar 11, 2019
@kravets-levko kravets-levko self-assigned this Mar 11, 2019
@kravets-levko kravets-levko requested review from arikfr and ranbena March 11, 2019 21:17
@ghost ghost added the in progress label Mar 11, 2019
@ranbena
Copy link
Contributor

ranbena commented Mar 11, 2019

Wow 1st look, looks so good! 👏
Great use of flexbox.

Few things I noticed:

  • System Status > Queues > Celery doesn't display a tooltip on hover.
  • Outdated Queries > "Last updated" value is "few seconds" on master, and "49 years ago" in this.
  • Outdated Queries - when there are no queries, no need to show PageSizeSelect and the "Sorry, we couldn't find anything." isn't a good fit for this situation.

@kravets-levko
Copy link
Collaborator Author

@ranbena Thanks! All fixed.

Copy link
Member

@arikfr arikfr left a comment

Choose a reason for hiding this comment

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

Cool 👍

See comments ->

client/app/components/admin/CeleryStatus.jsx Outdated Show resolved Hide resolved
client/app/components/admin/Layout.jsx Outdated Show resolved Hide resolved
client/app/components/admin/StatusBlock.jsx Outdated Show resolved Hide resolved
@arikfr
Copy link
Member

arikfr commented Mar 12, 2019

image

DB size was humanized before.

@kravets-levko kravets-levko requested a review from arikfr March 12, 2019 09:52
@ranbena
Copy link
Contributor

ranbena commented Mar 12, 2019

Suggestion for future - make these into Redash built-in dashboards (grids, counts). 🤘

@kravets-levko
Copy link
Collaborator Author

@ranbena That idea crossed my mind some time ago 🙂 We'll need some new visualization types for that.

client/app/components/admin/Layout.jsx Show resolved Hide resolved
client/app/components/admin/CeleryStatus.jsx Outdated Show resolved Hide resolved
@ranbena
Copy link
Contributor

ranbena commented Mar 12, 2019

@ranbena That idea crossed my mind some time ago 🙂 We'll need some new visualization types for that.

It could be a great forcing function to make better widgets and set an example to our clients.

@arikfr
Copy link
Member

arikfr commented Mar 12, 2019

It could be a great forcing function to make better widgets and set an example to our clients.

I agree. Let's start moving in this direction when our visualizations are pure React, so it's easier to create better APIs for them. :-)

@kravets-levko kravets-levko requested a review from ranbena March 12, 2019 16:29
@kravets-levko
Copy link
Collaborator Author

@ranbena (cc @arikfr) I fixed bug with forEach and refactored that page a bit more: sometimes it tried to call setState on unmounted component (I fixed that as well) and refined code - created more smaller components, and moved main logic to main (page) component.

Copy link
Contributor

@ranbena ranbena left a comment

Choose a reason for hiding this comment

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

LGTM.
@kravets-levko I don't have queue data but consider <Tabs animated={false}> if the tab change content animation is too crazy looking with actual data.

@kravets-levko
Copy link
Collaborator Author

kravets-levko commented Mar 12, 2019

@ranbena Which tabs do you mean? There are top-level tabs ("System Status", "Celery Status", ...) and inner tabs on "Celery Status" tab. For top-level tabs I had to disable animation because that tabs are link, and when clicked - animation started, then component destroyed - so it looked really weird. For inner tabs animation doesn't look so weird.

@ranbena
Copy link
Contributor

ranbena commented Mar 12, 2019

@ranbena Which tabs do you mean?

Celery tabs

For inner tabs animation doesn't look so weird.

Okey dokey

@kravets-levko
Copy link
Collaborator Author

Decided to disable animation - for consistency between top-level and inner tabs.

@kravets-levko
Copy link
Collaborator Author

@arikfr @gabrieldutra If you're fine with this - LMK if I can merge it. Thanks!

@kravets-levko kravets-levko merged commit 73c8e30 into getredash:master Mar 27, 2019
@kravets-levko kravets-levko deleted the admin-pages branch March 28, 2019 08:12
harveyrendell pushed a commit to pushpay/redash that referenced this pull request Nov 14, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Frontend: React Frontend codebase migration to React Frontend
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants