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 Table visualization to React Part 2: Editor #4175

Merged
merged 16 commits into from
Oct 24, 2019
Merged

Conversation

kravets-levko
Copy link
Collaborator

@kravets-levko kravets-levko commented Sep 23, 2019

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

  • Refactor

Description

  • Convert component to React
    • Columns tab
      • common options
      • options specific to each column type
      • drag-and-drop column sorting
    • Grid tab
  • Debounce inputs
  • Cleanup
  • Tests

Related Tickets & Documents

#3301 (Migrate Visualizations to React -> Table)

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

image
image

@kravets-levko kravets-levko self-assigned this Sep 23, 2019
@arikfr arikfr mentioned this pull request Sep 23, 2019
24 tasks
@kravets-levko kravets-levko marked this pull request as ready for review September 27, 2019 14:01
@kravets-levko
Copy link
Collaborator Author

I need to add some tests, but Editor is completely migrated and its code is ready for review

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.

@kravets-levko code is great, see a few comments

client/app/visualizations/table/columns/datetime.js Outdated Show resolved Hide resolved
client/app/visualizations/table/columns/datetime.js Outdated Show resolved Hide resolved
client/app/visualizations/table/Editor/editor.less Outdated Show resolved Hide resolved
client/app/visualizations/table/Editor/ColumnEditor.jsx Outdated Show resolved Hide resolved
dateTimeFormat: PropTypes.string,
}).isRequired,
onChange: PropTypes.func.isRequired,
};
Copy link
Contributor

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 :/

Copy link
Collaborator Author

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

Copy link
Contributor

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 😒

Copy link
Member

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

PR #4389

@ranbena
Copy link
Contributor

ranbena commented Sep 28, 2019

I've noticed that hovering over the <div className="m-b-15"> shows the move cursor but doesn't allow drag.

@kravets-levko
Copy link
Collaborator Author

I've noticed that hovering over the

shows the move cursor but doesn't allow drag.

It will be irrelevant when I'll implement a new columns UI (with collapsible rows)

@ranbena
Copy link
Contributor

ranbena commented Sep 30, 2019

@kravets-levko good job 👍

@kravets-levko
Copy link
Collaborator Author

@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

@kravets-levko kravets-levko mentioned this pull request Sep 30, 2019
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants