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

Improve legend error #1758

Merged
merged 9 commits into from
Oct 4, 2017
Merged

Improve legend error #1758

merged 9 commits into from
Oct 4, 2017

Conversation

rubenmoya
Copy link
Contributor

@rubenmoya rubenmoya commented Sep 19, 2017

Related to: #1715

The goal was to add an error message related to limits. After some research we realized we can't have limits errors here since the legends come from viz.json, it could only fail when instantiating the map or when parsing CartoCSS, and we're already covered.

The only change introduced by this PR is changing the No data available styles and refactor the legend-view-base.js.

screen shot 2017-09-19 at 10 42 31
screen shot 2017-09-19 at 10 43 30
screen shot 2017-10-03 at 12 06 50

Acceptance

To show the "Something went wrong" text:

  • Create a map with text and numeric values
  • Change the color of the points by value and choose a numeric column
  • Open the cartocss editor, and change the column name by a text column
  • The "Something went wrong" message with a placeholder should appear in the legend

To show the "No data available" text:

  • Not sure how we can reproduce it

this.$el.hide();
}
this.model.isVisible()
? this.$el.show()
Copy link
Contributor

Choose a reason for hiding this comment

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

😍

Copy link
Contributor

Choose a reason for hiding this comment

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

Or this.$el.toggle(this.model.isVisible()) but I prefer the way you implemented.

Copy link
Contributor

Choose a reason for hiding this comment

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

@nobuti comment is the winner, IMHO 💃 .


var placeholder = this._getPlaceholderHTML();

var error = this.model.isError()
Copy link
Contributor

Choose a reason for hiding this comment

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

no loading state?

Copy link
Contributor Author

@rubenmoya rubenmoya Sep 19, 2017

Choose a reason for hiding this comment

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

What a fail 😓 let me change it

@nobuti
Copy link
Contributor

nobuti commented Sep 22, 2017

Could we prepare it for the acceptance? I guess we will need a builder branch, right?

@rubenmoya
Copy link
Contributor Author

Builder branch created!

@rubenmoya
Copy link
Contributor Author

Deployed to ded15

@xavijam
Copy link
Contributor

xavijam commented Oct 4, 2017

Tested and working 👌 , merging guys!

@xavijam xavijam merged commit 0e4f870 into v4 Oct 4, 2017
@xavijam xavijam deleted the 1715-improve-legend-error branch October 4, 2017 12:23
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.

4 participants