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

More scattergl symbols #1482

Closed

Conversation

destradafilm
Copy link

@destradafilm destradafilm commented Mar 15, 2017

Added 49 more symbols to get as close to the 'scatter' plot symbol list to include :

circle
circle-open
square
square-open
diamond
diamond-open
cross
x
triangle-up
triangle-up-open
triangle-down
triangle-down-open
triangle-left
triangle-left-open
triangle-right
triangle-right-open
triangle-ne
triangle-ne-open
triangle-nw
triangle-nw-open
triangle-se
triangle-se-open
triangle-sw
triangle-sw-open
pentagon
pentagon-open
hexagon
hexagon-open
hexagon2
star
star-open
hexagram
diamond-tall
diamond-tall-open
hourglass
hourglass-open
bowtie
bowtie-open
circle-cross
circle-x
square-cross
square-x
cross-thin
asterisk

previous list:
screen shot 2017-03-16 at 10 07 05 am

updated list:
screen shot 2017-03-16 at 10 05 12 am

@destradafilm destradafilm changed the title Add 49 more unicode symbols for scattergl symbol types scattergl-on-par-shapes Mar 15, 2017
@etpinard
Copy link
Contributor

Thanks a lot!

We should make these on-par with the SVG symbol list or at the very least we shouldn't add scattergl-only symbols.

@etpinard
Copy link
Contributor

... and by the way please we don't need those devtools/ modifications as part of this PR.

@destradafilm
Copy link
Author

@etpinard Sounds good, I reverted the devtools folder back to master.

I attempted to find all the unicode symbols that I could to match the SVG symbol list, but couldn't find all of them, so this is the best I could do.

I'm wondering, would it make sense to render the symbols on the scattergl from the same SVGs? Or is there a reason it's using Unicode symbols?

@destradafilm
Copy link
Author

And, I actually counted wrong, there's 35 more symbols so a total of 43

'square-x': '⊠',
'cross-thin': '+',
'asterisk': '✱'
};
Copy link
Contributor

Choose a reason for hiding this comment

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

@destradafilm try running npm run lint-fix

@etpinard
Copy link
Contributor

I attempted to find all the unicode symbols that I could to match the SVG symbol list, but couldn't find all of them, so this is the best I could do.

That's ok. We don't have to have perfect parity between WebGL and SVG symbol. Like I said in #1482 (comment), we should just make sure that we don't introduce WebGL-only markers - which I think you did, but we should double check possible by adding a jasmine test.

cross: '+',
x: '❌'
};
cross: '➕',
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like you changed the cross symbol which is breaking the gl3d_marker_arrays and the gl3d_projection_traces mocks.

@@ -16,7 +16,7 @@ module.exports = {
'square-open': '□',
diamond: '◆',
'diamond-open': '◇',
cross: '',
cross: '+',
Copy link
Author

Choose a reason for hiding this comment

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

I changed cross back to original symbol, so that means I removed 'cross-thin'

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks. Reverting back to the current symbol is the way to go here.

That said, we could (and should I'd say) make cross thicker and add cross-thin in v2.

Copy link
Contributor

@etpinard etpinard Apr 3, 2017

Choose a reason for hiding this comment

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

Added to the #420 list of planned breaking changes.

@etpinard etpinard changed the title scattergl-on-par-shapes More scattergl symbols Mar 21, 2017
@etpinard
Copy link
Contributor

Ok @destradafilm before merging we should make the new WebGL symbols have image test support.

Can you try adding an scatter3d and scattergl data/layout mock in test/image/mock/ showing off all the new symbols?

If you're up for a challenge, you can also try generating the baseline images using our image test docker container.

@etpinard etpinard added this to the v1.26.0 milestone Mar 22, 2017
@etpinard
Copy link
Contributor

@destradafilm Are you ok with taking care of #1482 (comment) or do you me to do it? This would make a nice addition to next week's 1.26.0.

@destradafilm
Copy link
Author

@etpinard I've been too busy in the past few days to work on it. If you're willing to do it, that would be great! If not, it will probably have to be the end of next week before I can get to it.

@etpinard
Copy link
Contributor

@destradafilm no problem. I'll take care of it (should only be a 5-minute job)!

Thanks again for the PR 🍻

@etpinard
Copy link
Contributor

etpinard commented Apr 3, 2017

Superseded by #1550

@etpinard etpinard closed this Apr 3, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants