-
Notifications
You must be signed in to change notification settings - Fork 4.4k
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 Table visualization to React Part 2: Editor #4175
Conversation
I need to add some tests, but Editor is completely migrated and its code is ready for review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kravets-levko code is great, see a few comments
dateTimeFormat: PropTypes.string, | ||
}).isRequired, | ||
onChange: PropTypes.func.isRequired, | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Couldn't figure out why but if I create a new query select 1556668800000 as date
and then edit the column to be "date/time" it won't react to a format input. I enter "MMM-YYYY" but the viz preview stays the number :/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cast it to date/time type so redash will convert it to moment
instance. it's quite weird, but changing this behavior is not related to this PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ohhh yeah that's weird 😒
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's definitely confusing and not how the other types work -- let's open a separate issue for this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PR #4389
I've noticed that hovering over the |
It will be irrelevant when I'll implement a new columns UI (with collapsible rows) |
@kravets-levko good job 👍 |
@ranbena ATM I'm refactoring Parameters component a bit - was unable to re-use that animations, so decided to extract all that stuff to a reusable component. I'll open a PR in 10-20 minutes |
What type of PR is this? (check all applicable)
Description
Related Tickets & Documents
#3301 (Migrate Visualizations to React -> Table)
Mobile & Desktop Screenshots/Recordings (if there are UI changes)