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 Data Sources and Alert Destinations pages to React #3470

Merged
merged 59 commits into from
Mar 28, 2019

Conversation

gabrieldutra
Copy link
Member

@gabrieldutra gabrieldutra commented Feb 19, 2019

Description

Migrate Data Sources and Alert Destinations pages to React

Changes

General

  • Migrate TypePicker to React
  • Review TypePicker style

List Pages

  • Data Sources
  • Destinations
  • Improve behavior for empty set

Create Pages

  • Data Sources
    • Select Type
    • Configure Type
    • Done
  • Destinations
    • Select Type
    • Configure Type
    • Done

Edit Pages

  • Data Sources
  • Destinations

To Do

  • Routes /data_sources/new & /destinations/new auto-opening the modal
  • Help using HelpTrigger
  • Refine Edit forms
  • Update Cypress tests

Possible Extra

  • Add edit & delete actions to the cards on List Pages (ref)

Notes

Data Sources and Destinations are very similar pages, however I'm not sure how much logic I should share. They are sharing TypePicker (renamed to CardsList) and DynamicForm, but to me it doesn't feel that they should share much more. I'll keep the idea of refactoring parts of it to avoid code duplication anyway.

Preview

Overview

overview-dialog

@gabrieldutra gabrieldutra added Frontend Frontend: React Frontend codebase migration to React labels Feb 19, 2019
@gabrieldutra gabrieldutra self-assigned this Feb 19, 2019
@ghost ghost added the in progress label Feb 19, 2019
@gabrieldutra
Copy link
Member Author

All things considered I favor positioning the "Setup Instructions (?)" at the top right.

Perhaps right-aligned but not aside to the Name label? (I don't see a simple way to put it aside to it, also it makes some sense for it not to be there 😅)

right-aligned-help-trigger

I think the step titles should be either "Select type > Configure > Done" or "Type selection > Configuration > Done".

👍

@gabrieldutra
Copy link
Member Author

Thanks, @ranbena, my last concern about this is:

I could not make a better behavior for Data Source long names using Ant Cards (currently height is fixed for 3 lines and overflow is hidden). I'll appreciate if you have ideas for this 😁. The options I have in mind: - use javascript to do some better trick; - use the old visual-card view.

If you've already checked this and actually think it's an acceptable behavior, LMK 😅.

BTW, I had to revert cy.click to cy.wait as it didn't work. If this is really due to animation I think it won't make weird diffing, so it's safe to keep it until a better solution appears. (I've tried to use percy specific CSS with your idea as well)

@ranbena
Copy link
Contributor

ranbena commented Mar 15, 2019

I'll appreciate if you have ideas for this

Overflow hidden is good enough cause it prevents breakage and long names are probably an edge case (pun intended).
In addition you can add this, which will benefit the majority of our users:

.cards-list .cards-list-item h3 {
 ...
  display: -webkit-box;
  -webkit-line-clamp: 3;
  -webkit-box-orient: vertical;
}

BTW, I had to revert cy.click to cy.wait as it didn't work.

👍

@gabrieldutra
Copy link
Member Author

Thanks a lot @ranbena! 😁

I actually had to add /* autoprefixer: off */ for it to work (the box-orient was being removed even thought it's necessary to do the line-clamp). It improved the experience anyway.

If there's no further comments, this is ready to go when convenient. 👍

@ranbena
Copy link
Contributor

ranbena commented Mar 15, 2019

Go ahead and merge whenever you're ready.

@gabrieldutra
Copy link
Member Author

Conflicts resolved, gonna merge this big guy tomorrow 🚀

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.

When saving a data source it should open the data source page, so the user can do a Test Connection.

@ranbena
Copy link
Contributor

ranbena commented Mar 27, 2019

When saving a data source it should open the data source page, so the user can do a Test Connection.

But, if possible, without the autofocusing in that specific scenario, cause it's confusing ("I just created this, why should I need to edit it??").

@ranbena
Copy link
Contributor

ranbena commented Mar 27, 2019

Just a ux thought I want to share (out of scope):

Screen Shot 2019-03-27 at 18 37 08

@gabrieldutra
Copy link
Member Author

When saving a data source it should open the data source page, so the user can do a Test Connection.

👍

But, if possible, without the autofocusing in that specific scenario, cause it's confusing ("I just created this, why should I need to edit it??").

🤔, perhaps disable the autofocus for Edit Pages? It makes a lot of sense when it's a form for a new resource, but when editing I don't think it's that relevant (as edit pages also perform as a view page in this case)...

@gabrieldutra
Copy link
Member Author

gabrieldutra commented Mar 27, 2019

@arikfr & @ranbena,

I've updated it to redirect to the created resource. (Both Data sources and Alert destinations)

As for the autofocus, used to set the first input with it DynamicForm by default, I removed this and turned it into a field property. With this, I also removed autofocus for User Edit.

Please check my last commits 🙂

@gabrieldutra
Copy link
Member Author

Just a ux thought I want to share (out of scope):

Screen Shot 2019-03-27 at 18 37 08

I've been thinking about it and I actually don't like that much having an action button in a Notification, but I think an option would be to automatically run the connection test when creating a data source, then lead to 2 scenarios:

  • Connection success: Inform the user with a notification and save it;
  • Connection failure: Show the error with a dialog and offer to either Save it anyway or go back to edit setting data;

This could be in the Creation Dialog as a new step as well.

@ranbena
Copy link
Contributor

ranbena commented Mar 28, 2019

  • Connection success: Inform the user with a notification and save it;
  • Connection failure: Show the error with a dialog and offer to either Save it anyway or go back to edit setting data;

YES

This could be in the Creation Dialog as a new step as well.

YESSSS

@gabrieldutra plz open a forum topic and I'll submit a design.

@arikfr
Copy link
Member

arikfr commented Mar 28, 2019

Good to go.

I have to admit that I'm not 100% sold on the new dialog interface: it feels cramped and makes the different types less discoverable/visible. Let's move on, but keep this in mind as something to revisit.

@arikfr arikfr merged commit f9cc230 into getredash:master Mar 28, 2019
harveyrendell pushed a commit to pushpay/redash that referenced this pull request Nov 14, 2019
…#3470)

* Migrate TypePicker to React

* Migrate DataSources and Destinations List

* Fixes to DestinationsList

* Add CreateDataSource (testing with Steps)

* Render the form after type selection

* Add HELP_LINKS to CreateDataSource

* Add Done behavior

* Add scrollToTop to CreateDataSource

* TypePicker styling adjusts

* Add CreateDestination

* Update resouce gets to componentDidMount

* Create EditForm components

* Migrate Edit pages

* Remove angular logic from DynamicForm

* Add actions to EditPages

* TypePicker title style adjustments

* Add Empty and Loading state

* UX improvements

* Review changes

* Styling updates on TypePicker, forms background fix

* Add blank line removed by mistaken

* Reorganize TypePicker

* Hide Search on List Pages

* Fix spacing in Forms

* Update Create Data Source and Destination to be a Dialog

* Remove max-height from the form

* Fix DynamicForm import in CreateUserDialog

* Route /new to open CreateSourceDialog

* Add HelpTrigger + refine styling and Edit Pages

* Remove help links from data source resource

* Update Cypress specs

* TypePicker -> CardsList

* Remove old TypePicker styling and change CardsList styling to less

* Test if Percy shows Dialogs

* Personal review cleanup

* CR

* Remove unnecessary query on dialog success

* Handle resource errors in Edit Pages

* Add CreateDestination policy

* Add placeholder and separator to the Name field

* Use cy.click instead of cy.wait

* Revert "Use cy.click instead of cy.wait" (Didn't work)

This reverts commit 77285d9.

* Align help trigger on the right and rename Steps

* Refine behavior for long names

* Update toastr calls to use notification instead

* Redirect to target after creation

* Remove autoFocus on DynamicForm for Edit Pages

* Add eslint-disable for cy.wait
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.

4 participants