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

Format attribute for widgets, popups and legends [ch60845] #1626

Merged
merged 17 commits into from
May 12, 2020

Conversation

neokore
Copy link
Contributor

@neokore neokore commented Apr 28, 2020

const options = { format, config, dynamic };
const formatString = legend.format;
const formatFunc = formatString
? (value) => format$1(value, formatString)

Choose a reason for hiding this comment

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

Misleading line break before '?'; readers may interpret this as an expression boundary.

padding = length < width ? new Array(width - length + 1).join(fill) : "";

// If the fill character is "0", grouping is applied after padding.
if (comma && zero) value = group(padding + value, padding.length ? width - valueSuffix.length : Infinity), padding = "";

Choose a reason for hiding this comment

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

Expected an assignment or function call and instead saw an expression.

// Break the formatted value into the integer “value” part that can be
// grouped, and fractional or exponential “suffix” part that is not.
if (maybeSuffix) {
i = -1, n = value.length;

Choose a reason for hiding this comment

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

Expected an assignment or function call and instead saw an expression.

else if (!formatTypes[type]) precision === undefined && (precision = 12), trim = true, type = "g";

// If zero fill is specified, padding goes after sign and before digits.
if (zero || (fill === "0" && align === "=")) zero = true, fill = "0", align = "=";

Choose a reason for hiding this comment

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

Expected an assignment or function call and instead saw an expression.

if (type === "n") comma = true, type = "g";

// The "" type, and any invalid type, is an alias for ".12~g".
else if (!formatTypes[type]) precision === undefined && (precision = 12), trim = true, type = "g";

Choose a reason for hiding this comment

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

Expected an assignment or function call and instead saw an expression.

+ this.symbol
+ (this.zero ? "0" : "")
+ (this.width === undefined ? "" : Math.max(1, this.width | 0))
+ (this.comma ? "," : "")

Choose a reason for hiding this comment

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

Misleading line break before '+'; readers may interpret this as an expression boundary.

+ this.sign
+ this.symbol
+ (this.zero ? "0" : "")
+ (this.width === undefined ? "" : Math.max(1, this.width | 0))

Choose a reason for hiding this comment

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

Misleading line break before '+'; readers may interpret this as an expression boundary.

+ this.align
+ this.sign
+ this.symbol
+ (this.zero ? "0" : "")

Choose a reason for hiding this comment

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

Misleading line break before '+'; readers may interpret this as an expression boundary.

return this.fill
+ this.align
+ this.sign
+ this.symbol

Choose a reason for hiding this comment

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

Misleading line break before '+'; readers may interpret this as an expression boundary.

FormatSpecifier.prototype.toString = function() {
return this.fill
+ this.align
+ this.sign

Choose a reason for hiding this comment

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

Misleading line break before '+'; readers may interpret this as an expression boundary.

Copy link
Member

@Jesus89 Jesus89 left a comment

Choose a reason for hiding this comment

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

You could install a linter locally (in VS) to avoid Hound messages for issues with the linter in JS.
It seems that tests for map, widgets, legends, and popups are failing / not updated.

@Jesus89
Copy link
Member

Jesus89 commented Apr 29, 2020

It seems that the linter issue is only with the bundle because of the new dependency d3/format. So we must exclude the bundle from the linter check.

const options = { format, config, dynamic };
const formatString = legend.format;
const formatFunc = formatString
? (value) => format(value, formatString)

Choose a reason for hiding this comment

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

Misleading line break before '?'; readers may interpret this as an expression boundary.

@@ -1,7 +1,8 @@
from ..widget import Widget


def histogram_widget(value, title=None, description=None, footer=None, read_only=False, buckets=20, weight=1):
def histogram_widget(value, title=None, description=None, footer=None, read_only=False,
buckets=20, weight=1):
Copy link
Contributor

Choose a reason for hiding this comment

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

why this line CR into 2 lines?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is to avoid too long line warning

@VictorVelarde
Copy link
Contributor

The python side look good to me. Let's see if we can get travis to pass

@neokore
Copy link
Contributor Author

neokore commented Apr 29, 2020

Pushed changes for Hound and Travis CI.

@neokore neokore marked this pull request as ready for review April 30, 2020 11:28
@neokore
Copy link
Contributor Author

neokore commented Apr 30, 2020

Travis forced to rebuild and now it passes. A ✔️ for this?

" popup_hover=[\n",
" popup_element('name', 'Country', 'population')\n",
" popup_element('name', 'City'),\n",
" popup_element('pop_max', title='Rounded population', format=',.2r')\n",
Copy link
Contributor

Choose a reason for hiding this comment

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

LGTM. I'm gonna ping here @manmorjim as he's interested in popup and formatting. It looks like using d3.format on both CF and web-sdk could be nice combination

Copy link
Contributor

@VictorVelarde VictorVelarde left a comment

Choose a reason for hiding this comment

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

New additions on format LGTM.

@Jesus89 Jesus89 merged commit 7e0e8a4 into develop May 12, 2020
@Jesus89 Jesus89 deleted the feature/ch60845/format-attribute branch May 12, 2020 07:01
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.

5 participants