Skip to content

Conversation

@dmt0
Copy link
Contributor

@dmt0 dmt0 commented Aug 9, 2018

geo fixes:

  • axes panel - visibility issues and missing widgets (feature parity stuff)
  • correct options for fill dropdown
  • StyleTracePanel sections missing for atlas maps

super(props, context);
const {localize: _} = context;

// Replicated so that strings can be statically analyzed for localization
Copy link
Contributor

Choose a reason for hiding this comment

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

why is replication needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My guess was that there's a script that does static analysis of the code and adds strings to the translation files, so this thing needs to live inside a component. Tried putting it in one and accessing from another - like UnconnectedFilterValue.operation - no luck. What's the way?

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.

you could make it a function? and pass in localize as a prop?

<Numeric label={_('Scale')} attr="projection.scale" min={0} />
</PlotlySection>

<PlotlySection name={_('Map Rotation')}>
Copy link
Contributor

Choose a reason for hiding this comment

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

I would key frame and rotation at the bottom, personally. not sure if anyone knows enough math to actually edit the rotation numbers tho

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it's there in cloud ya know, and I thought to put all relevant things together, everything below has to do with map contents...

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah let's make map contents the top stuff please, and leave the weird stuff at the bottom

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

Copy link
Contributor

Choose a reason for hiding this comment

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

OK but the region and projection should stay at the top :) the Frame and Rotation and Scale and Resolution should be at the bottom :)

@dmt0 dmt0 requested a review from VeraZab August 9, 2018 16:54
@nicolaskruchten
Copy link
Contributor

not sure the error messages really need to be localized, but I appreciate the thoroughness :)

@dmt0
Copy link
Contributor Author

dmt0 commented Aug 9, 2018

no idea what percy is doing, I see all panels just fine locally

attr="resolution"
options={[
{label: _('1:110,000,000'), value: 110},
{label: _('1:50,000,000'), value: 50},
Copy link
Contributor

Choose a reason for hiding this comment

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

that label title is a little weird no?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

that's what is in cloud - most importantly it will be translated to all languages :)

@VeraZab
Copy link
Contributor

VeraZab commented Aug 9, 2018

💃 on the code

@nicolaskruchten
Copy link
Contributor

💃 once "map region" is back at the top :)

@nicolaskruchten
Copy link
Contributor

err hang on, weirdness in Percy.

/>
<Numeric label={_('Thickness')} attr="gridwidth" units="px" />
<ColorPicker label={_('Color')} attr="gridcolor" />
<Numeric label={_('Reference')} attr="tick0" units="deg" />
Copy link
Contributor

Choose a reason for hiding this comment

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

these are problematic because they share attr names with other stuff in cartesian!

Copy link
Contributor

Choose a reason for hiding this comment

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

do a search for dtick and you'll see

Copy link
Contributor

Choose a reason for hiding this comment

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

see also #535 which is the same-ish issue: attrs with the same name in different trace-type/axes that should have different labels/treatment

@dmt0 dmt0 merged commit 3727c27 into master Aug 9, 2018
@dmt0 dmt0 deleted the geo-fixes branch August 9, 2018 19:58
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.

Switching from variable to constant should hide colorscale ensure that trace group names are localized Geo Layout Many non-localized labels

4 participants