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

[explore] convert query and save btns to react #690

Merged
merged 2 commits into from
Jul 8, 2016
Merged

[explore] convert query and save btns to react #690

merged 2 commits into from
Jul 8, 2016

Conversation

ascott
Copy link

@ascott ascott commented Jun 29, 2016

first pass at moving explore.js from jquery to react. moves query and save btns to react. modal is still using the dom api. will move the modal to react in a subsequent pr.

plz review @mistercrunch @georgeke

data-target="#save_modal"
data-toggle="modal"
>
<i className="fa fa-plus-circle"></i>Save as
Copy link
Author

Choose a reason for hiding this comment

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

need to figure out a pattern for getting translated strings into js.

Copy link
Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

We use Python's Babel on the backend, hopefully we can have a consistent take on translations where all strings to be translated are all in the same framework.
http://babel.pocoo.org/en/latest/

Copy link
Author

Choose a reason for hiding this comment

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

right, we should definitely continue to use babel. i should write a helper so we have a consistent way to pass translated text from the server to bootstrap the ui.

Copy link
Author

@ascott ascott Jul 1, 2016

Choose a reason for hiding this comment

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

something like:

server:

bootstrap_data("phrase_bundle_name", {
  phrase_key: _("phrase_key"),
})

client:

BootstrapData.get('phrase_bundle_name');
// returns json blob with keys/translated phrases

@coveralls
Copy link

coveralls commented Jun 29, 2016

Coverage Status

Coverage remained the same at 81.085% when pulling 14d0dc872ea3b8ae0882bda45f059fc48ec982e9 on ascott:alanna-explore-query-save-btns-to-react into dbb9356 on airbnb:master.

@mistercrunch
Copy link
Member

The travis build failed. You can see it's logs for details.

});
const queryAndSaveBtnsEl = document.getElementById('js-query-and-save-btns');
ReactDOM.render(
<QueryAndSaveBtns
Copy link
Member

Choose a reason for hiding this comment

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

this the line that failed, I think webpack needs a .jsx extension to get the right loader.

@coveralls
Copy link

coveralls commented Jul 1, 2016

Coverage Status

Coverage increased (+0.01%) to 81.097% when pulling e9ae1a96fc5491def058a398ea1456548450c6b5 on ascott:alanna-explore-query-save-btns-to-react into dbb9356 on airbnb:master.

@mistercrunch
Copy link
Member

It passed the build now, needs rebase

@coveralls
Copy link

coveralls commented Jul 6, 2016

Coverage Status

Coverage remained the same at 81.036% when pulling 428cd9d8300954c706f82b3f9445d4929b997151 on ascott:alanna-explore-query-save-btns-to-react into 8135c24 on airbnb:master.

@coveralls
Copy link

coveralls commented Jul 7, 2016

Coverage Status

Coverage remained the same at 81.036% when pulling a5b8ef69ed9b81369c328542a6859da6a7bfb4db on ascott:alanna-explore-query-save-btns-to-react into 8135c24 on airbnb:master.

@coveralls
Copy link

coveralls commented Jul 7, 2016

Coverage Status

Coverage remained the same at 81.036% when pulling 51805b1 on ascott:alanna-explore-query-save-btns-to-react into 8135c24 on airbnb:master.

@coveralls
Copy link

coveralls commented Jul 7, 2016

Coverage Status

Coverage remained the same at 81.036% when pulling 51805b1 on ascott:alanna-explore-query-save-btns-to-react into 8135c24 on airbnb:master.

@coveralls
Copy link

coveralls commented Jul 7, 2016

Coverage Status

Coverage remained the same at 81.036% when pulling 51805b1 on ascott:alanna-explore-query-save-btns-to-react into 8135c24 on airbnb:master.

@georgeke
Copy link
Contributor

georgeke commented Jul 7, 2016

Would it make more sense for explore.jsx to be in the explore/ directory as well? That's what I'm doing for dashboard.jsx

@georgeke
Copy link
Contributor

georgeke commented Jul 7, 2016

LGTM once we pass the $('#btn_modal_save').click function as the onSave prop.

@ascott
Copy link
Author

ascott commented Jul 8, 2016

thanks for the review @georgeke. good suggestion on moving explore.jsx to the explore folder too.

the $('#btn_modal_save') element is referring to the save button in the modal.

screenshot 2016-07-07 18 11 54

the save button that i'm refactoring here triggers opening the modal, which is happening via the bootstrap modal dom api on these lines:
https://github.com/airbnb/caravel/pull/690/files#diff-57c9445f7f4fbcf566d006ada2062698R23

screenshot 2016-07-07 18 12 01

we will definitely want to refactor the modal as well in another PR.

@coveralls
Copy link

coveralls commented Jul 8, 2016

Coverage Status

Coverage remained the same at 81.036% when pulling d3fda53 on ascott:alanna-explore-query-save-btns-to-react into 8135c24 on airbnb:master.

@ascott ascott merged commit 8020464 into apache:master Jul 8, 2016
@ascott ascott deleted the alanna-explore-query-save-btns-to-react branch July 8, 2016 01:39
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 0.11.0 labels Feb 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 0.11.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants