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

Added url shortner for sharing query link #1314

Merged
merged 5 commits into from
Oct 25, 2016

Conversation

vera-liu
Copy link
Contributor

@vera-liu vera-liu commented Oct 11, 2016

Addressing comments from today's meeting

Done:

  • Put url shortener inside CopyToClipboard component
  • Delete some unused props and functions

needs-review @ascott @bkyryliuk @mistercrunch

@mistercrunch
Copy link
Member

build doesn't go through...

onMouseOut() {
// delay to avoid flash of text change on tooltip
setTimeout(this.resetTooltipText, 200);
}

getShortUrl() {
Copy link
Member

@mistercrunch mistercrunch Oct 11, 2016

Choose a reason for hiding this comment

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

Seems like CopyToClipboard should be more generic, isn't it used in other places that have nothing to do with shortening URLs?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved the shortner outside CopyToClipboard to components separately in new commit

if (qe.autorun) params.push('autorun=' + qe.autorun);
if (qe.sql) params.push('sql=' + qe.sql);

return getLink(this.state.cleanUri, params);
Copy link
Member

Choose a reason for hiding this comment

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

I'm trying to understand the new logic and I don't see where the url gets crafted before sending to the backend for shortening.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry this was moved to CopyQueryTabUrl in the previous PR, But I forgot to remove it in this component. It's not used in this component anymore.

@vera-liu
Copy link
Contributor Author

Thanks for reviewing! I moved shortUrl out of CopyToClipboard and the ajax call to common.js. @mistercrunch

@@ -29,7 +29,7 @@ class TabbedSqlEditors extends React.Component {
const queryEditorProps = {
id: shortid.generate(),
title: getParamFromQuery(this.state.query, 'title'),
dbId: getParamFromQuery(this.state.query, 'dbid'),
dbId: parseInt(getParamFromQuery(this.state.query, 'dbid'), 10),
Copy link
Contributor Author

@vera-liu vera-liu Oct 12, 2016

Choose a reason for hiding this comment

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

Previously the database was not set in DatabaseSelect when loading the shared url because the passed string failed the prop as integer. Fixed it here


return queryLink;
onShortUrlSuccess(data) {
Copy link
Member

Choose a reason for hiding this comment

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

looks like you are not handling error case here, what setting the state directly in the componentWillMount ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

for the error case the state won't get updated, so the shortUrl stays as empty string.


return queryLink;
onShortUrlSuccess(data) {
Copy link
Contributor

Choose a reason for hiding this comment

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

i'm a big fan of more, smaller methods, rather than putting everything in line. this seems cleaner than putting in directly in componentWillMount, imo.

@ascott
Copy link
Contributor

ascott commented Oct 24, 2016

generally lgtm. i would use git's rebase functionality for syncing your branch, which will avoid having many merge commits here.

git pull --rebase upstream master

@vera-liu vera-liu merged commit 6f1e7c3 into apache:master Oct 25, 2016
@vera-liu vera-liu deleted the vliu_share_query_shortner branch November 1, 2016 19:01
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 0.12.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.12.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants