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

Feature: Improved chart editor UI/UX #672

Merged
merged 1 commit into from Nov 30, 2015
Merged

Conversation

alonho
Copy link
Contributor

@alonho alonho commented Nov 24, 2015

Here's a glimpse:

screen shot 2015-11-24 at 9 55 22 am

Things I did:

  1. Added ui-select widgets for selecting x/y/dimension.
  2. Refactored the hell out of the chart-editor directive (it now has an isolated scope).
  3. Changed some names (column chart -> bar chart) while keeping the model backward compatible.
  4. Add a name label to all axes.

Things I'm unsure of:

  1. When a query returns with other columns that these defined in the widgets they are cleared. Would it be annoying for the user? (imagine a situation where you change the query and it returns 0 rows, it then clears all the widgets).
  2. I changed stacking types names to: disabled, enabled and relative. Not sure if it's better than before.
  3. The layout of the widgets isn't perfect (the space to the right of the "Y Columns" is bothering my OCD).

@arikfr
Copy link
Member

arikfr commented Nov 24, 2015

About your questions:

  1. I think it should retain the configuration regardless of the query data. Is there a reason to clear it?
  2. Enabled/disabled sounds good. Relative might be confusing.
  3. Yes that whole area can be improved somehow, I just don't have a better solution for now :)

Some other comments:

  1. How about we name Y and Y2 and "Left Y Axis" and "Right Y Axis"?
  2. "Dimension" -> "Group By"?
  3. Maybe there should be a hint that the series are draggable?

@arikfr
Copy link
Member

arikfr commented Nov 24, 2015

Also maybe somehow emphasize that you need to start from selecting X column and Y column(s)?
Although this can be left for a future iteration.

@alonho alonho force-pushed the chart_editor branch 2 times, most recently from c1577c0 to 22a881b Compare November 24, 2015 15:22
@alonho
Copy link
Contributor Author

alonho commented Nov 24, 2015

Accepted most of your suggestions (left/right y axis, group by, hint for draggable (attached pic)).

Regarding requirements for X and Y. They are both required therefore are the only ones colored in red initially:

screen shot 2015-11-24 at 5 22 21 pm

screen shot 2015-11-24 at 5 05 55 pm

@alonho
Copy link
Contributor Author

alonho commented Nov 24, 2015

Regarding the reset of the widgets in case the query doesn't return the columns. I think there's a tradeoff:

  1. With the feautre, the user knows when no chart is showing and it's because he changed the query and should re-assign x or y columns (they become red).
  2. The downside is that if a query returns no rows, the widgets are reset, it doesn't necessarily mean the returned columns changed.
    perhaps a compromise is to reset the widgets only if more than one row was returned..

@arikfr
Copy link
Member

arikfr commented Nov 25, 2015

It definitely shouldn't reset the columns mapping if no rows returned, because this happens from time to time (for example: you try to refactor the query and mess something up) and can be very annoying to the user.

If the mapped columns are missing, we can remove them from the configuration. This seems reasonable.

@alonho
Copy link
Contributor Author

alonho commented Nov 25, 2015

I liked the compromise (: done.

@alonho
Copy link
Contributor Author

alonho commented Nov 25, 2015

The x scale is now automatically selected according to the column type.

@arikfr
Copy link
Member

arikfr commented Nov 26, 2015

About $scope.dateRangeEnabled not reloading after changing the xAxis type -- I think that we can turn it into a function, and check it in runtime (instead of adding a watch).

@alonho alonho force-pushed the chart_editor branch 4 times, most recently from d049f6b to a41d441 Compare November 27, 2015 00:44
@alonho
Copy link
Contributor Author

alonho commented Nov 27, 2015

Reverted the x scale automatic selection since it broke existing queries.

@alonho alonho force-pushed the chart_editor branch 2 times, most recently from a282b6b to 2c48c47 Compare November 30, 2015 08:03
@arikfr arikfr changed the title #671: Improve chart editor UI/UX Feature: Improved chart editor UI/UX Nov 30, 2015
arikfr added a commit that referenced this pull request Nov 30, 2015
Feature: Improved chart editor UI/UX
@arikfr arikfr merged commit b7720f7 into getredash:master Nov 30, 2015
@arikfr
Copy link
Member

arikfr commented Nov 30, 2015

Merged. 🎉

@alonho
Copy link
Contributor Author

alonho commented Nov 30, 2015

👯

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.

None yet

2 participants