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 conic gradient examples #1265

Merged
merged 4 commits into from Dec 19, 2018

Conversation

Projects
None yet
3 participants
@estelle
Copy link
Contributor

estelle commented Dec 11, 2018

estelle added some commits Dec 11, 2018

@wbamberg wbamberg assigned wbamberg and unassigned wbamberg Dec 14, 2018

@wbamberg wbamberg self-requested a review Dec 14, 2018

@kevinsimper
Copy link
Contributor

kevinsimper left a comment

Nice with examples for conic-gradient, could the first example be the simplest input you can give to the function? The first example contains 6 different arguments in hsl, could that be simplified down to 2 colors in HEX instead and still have that example longer down the examples?

@wbamberg
Copy link
Member

wbamberg left a comment

This works and looks great. I had a couple of comments on the content, though, and there's another issue about how the editor handles browser support.

On content:

  • I think if we possibly can we should avoid horizontal scrollbars in the source panes. With a browser window width of 1440px, we get about 62 characters in the source panes before we get horizontal scrollbars. The obvious way to avoid this is just line breaks:
background: conic-gradient(from 3.1416rad at 10% 50%,
    hsl(0, 100%, 50%), hsl(60, 100%, 50%),
    hsl(120, 100%, 50%), hsl(180, 100%, 50%),
    hsl(240, 100%, 50%), hsl(360, 100%, 50%));

The problem then of course is vertical scrollbars, which we also want to avoid :). But in this case actually it looks like we have enough vertical space that we can make options 2 and 3 multiline, and thereby avoid all scrollbars.

  • I think it's OK for some of the choices to show more complex syntax forms, but ideally at least some, and definitely the default-selected one, should show a really simple form that helps me understand the basic concept quickly. One problem with the current default background: conic-gradient(#e66465, #9198e5); is that I don't know off the top of my head what #e66465 looks like, so it would be better to use a named color. Actually the first example in the docs page: background: conic-gradient(red, orange, yellow, green, blue); is nice I think, and just seeing what it looks like is a good introduction to this function. Also maybe the second example could use only at and not from, to introduce these elements independently, e.g. : background: conic-gradient(at 50% 80%, #f69d3c, 10deg, #3f87a6, 350deg, #ebf8e1); or something like that.

The other problem here is that the example doesn't seem to work on Firefox - I don't see any output. Is this function not supported in Firefox? The interactive editor doesn't work very well for unsupported items, and we have an issue for this (#532) but it's not a high priority because it doesn't affect very many pages, because cross-browser support is in general pretty good. For now we just don't include examples for items that don't have good cross-browser support.

(There is another issue in here, where the technique we use to tell the editor whether an item is unsupported won't work in this particular case, because it only works on a per-property basis, not for particular values of properties. But given that the UX for unsupported items is so bad even when we can reliably detect it, I don't think that is worth worrying about today.)

@estelle
Copy link
Contributor Author

estelle left a comment

changed the first example to named colors, moved the hex to the third example

@estelle estelle referenced this pull request Dec 18, 2018

Open

DDN 64: Conic Gradients #658

14 of 14 tasks complete
@wbamberg
Copy link
Member

wbamberg left a comment

Thanks @estelle !

@wbamberg wbamberg merged commit 4f6e4f1 into mdn:master Dec 19, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
security/snyk - package.json (schalkneethling) No new issues
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment