Save modal component for explore v2 #1612
Conversation
vera-liu
force-pushed
the
vliu_reactify_save_modal
branch
from
8a8771e to
bf227e2
Compare
|
|
||
| fetchDashboards() { | ||
| const url = '/dashboardmodelviewasync/api/read?_flt_0_owners=' + this.props.user_id; | ||
| $.get(url, function (data) { |
There was a problem hiding this comment.
looks like we're duplicating this $.get here as well: https://github.com/airbnb/superset/pull/1612/files#diff-cb97812fa1c826050e9d4b6d69438938R119
we should move this fetching logic to the actions file.
There was a problem hiding this comment.
do we want to keep dashboards in global store? right now we only use them in SaveModal, I'm not sure if we would use them in other components.
There was a problem hiding this comment.
we may not use this action in other components yet, but it's great to keep all the fetching logic in one place and keep the components leaner. could be an opportunity to start an actions file for dashboard actions, but i think we could put this fetch in the explore actions for now.
| } | ||
| this.setState({ dashboards: choices }); | ||
| }.bind(this)); | ||
| $('input[name=add_to_dash]').change(setButtonsState); |
There was a problem hiding this comment.
seems like we have a lot of jquery dom manipulation happening in this component. can we do this in a more react-y way and avoid using jquery for dom manipulation?
| return { type: SET_FIELD_VALUE, key, value, label }; | ||
| } | ||
|
|
||
| export const SAVE_SLICE_SUCCEEDED = 'SAVE_SLICE_SUCCEEDED'; |
There was a problem hiding this comment.
do we need SAVE_SLICE_STARTED to show feedback to the user while we are saving?
| constructor(props) { | ||
| super(props); | ||
| this.state = { | ||
| save_to_dashboard_id: null, |
|
would love to see all async/fetching logic in an actions file and would love to see less jquery dom manipulation in favor of react patterns in the save modal component. otherwise, looking good! |
vera-liu
force-pushed
the
vliu_reactify_save_modal
branch
2 times, most recently
from
0af1517 to
706b7d5
Compare
|
Done:
PTAL @ascott |
vera-liu
force-pushed
the
vliu_reactify_save_modal
branch
from
2fbe00d to
f6ba5bf
Compare
| @@ -0,0 +1,249 @@ | |||
| /* eslint camel-case: 0 */ | |||
| import React, { PropTypes } from 'react'; | |||
| const $ = window.$ = require('jquery'); | |||
There was a problem hiding this comment.
can we use import $ from 'jquery'; here? don't think we need window.$ in this case.
| /> | ||
| </Alert> | ||
| } | ||
| <form> |
There was a problem hiding this comment.
would be great to use the react-bootstrap form components here, which we are using for the control panel container too: https://react-bootstrap.github.io/components.html#forms
|
yes! this looks so much better! it'll be easier to maintain/iterate on in the future! just one comment about using react-bootstrap forms, could do that in this PR or another, otherwise LGTM! |
vera-liu
force-pushed
the
vliu_reactify_save_modal
branch
from
609750b to
71b420a
Compare
mistercrunch
added
🏷️ bot
Done:
Todo:
needs-review @mistercrunch @ascott