Can parse CSS rule code contexts, too. #21

Merged
merged 1 commit into from Nov 28, 2015

Conversation

Projects
None yet
4 participants
@carljm
Contributor

carljm commented Nov 19, 2015

Hi! We'd like to use SassDoc as part of a "living styleguide" system for our projects, and for this to work it needs to have the capability to annotate CSS rule blocks as well as functions/variables/mixins/placeholders. This pull request adds that capability to the comments context parser.

I see that the broad concept has been rejected before, on "do just one thing" grounds (e.g. see SassDoc/sassdoc#268), but I'd like to request re-consideration. Accepting this small PR does not mean changing the primary supported purpose of SassDoc in any way, it just adds a bit more flexibility for those of us who want to experiment with extending SassDoc in "off the beaten path" ways, without harming or impacting the existing uses at all.

Thoughts? Thanks for considering.

@HugoGiraudel

This comment has been minimized.

Show comment
Hide comment
@HugoGiraudel

HugoGiraudel Nov 19, 2015

Member

For some reason, I didn’t get notified for this PR so I’ll ping other core contributors as well: @valeriangalliat @FWeinb @pascalduez. I will reply to this tomorrow as soon as I get to work. :)

Member

HugoGiraudel commented Nov 19, 2015

For some reason, I didn’t get notified for this PR so I’ll ping other core contributors as well: @valeriangalliat @FWeinb @pascalduez. I will reply to this tomorrow as soon as I get to work. :)

+ assert.deepEqual(context, {
+ type: 'css',
+ name: '.foo',
+ value: 'font-weight: bold;\n color: red;\n .bar {\n color: blue;\n }'

This comment has been minimized.

@valeriangalliat

valeriangalliat Nov 19, 2015

Member

Missing leading {

@valeriangalliat

valeriangalliat Nov 19, 2015

Member

Missing leading {

This comment has been minimized.

@valeriangalliat

valeriangalliat Nov 19, 2015

Member

Also maybe makes more sense to call it code instead of value.

@valeriangalliat

valeriangalliat Nov 19, 2015

Member

Also maybe makes more sense to call it code instead of value.

This comment has been minimized.

@carljm

carljm Nov 20, 2015

Contributor

Yeah, I debated about the names. I chose name and value for consistency with all the other types, but if that consistency doesn't matter it would probably be better to use selector and body.

And the leading { and trailing } of the body are both removed, but if you think it's better to include them I can certainly do that.

@carljm

carljm Nov 20, 2015

Contributor

Yeah, I debated about the names. I chose name and value for consistency with all the other types, but if that consistency doesn't matter it would probably be better to use selector and body.

And the leading { and trailing } of the body are both removed, but if you think it's better to include them I can certainly do that.

This comment has been minimized.

@carljm

carljm Nov 20, 2015

Contributor

(It looks like the trailing } is there, but it's not -- that's actually part of a nested rule.)

@carljm

carljm Nov 20, 2015

Contributor

(It looks like the trailing } is there, but it's not -- that's actually part of a nested rule.)

This comment has been minimized.

@valeriangalliat

valeriangalliat Nov 20, 2015

Member

Ahh, indeed, just the fact that the code was ending with a } and not beginning with one was bugging me.

And the {} are removed for other annotations so you're right that's the right way to do. 👍

The mixin uses name and code so it's still consistent to use code for CSS rules too, though selector and body also really make sense... let's see @SassDoc/owners's opinion.

@valeriangalliat

valeriangalliat Nov 20, 2015

Member

Ahh, indeed, just the fact that the code was ending with a } and not beginning with one was bugging me.

And the {} are removed for other annotations so you're right that's the right way to do. 👍

The mixin uses name and code so it's still consistent to use code for CSS rules too, though selector and body also really make sense... let's see @SassDoc/owners's opinion.

@@ -147,6 +147,13 @@ var scssContextParser = (function () {
end : lineNumberFor(endIndex) + 1
};
}
+ } else {

This comment has been minimized.

@valeriangalliat

valeriangalliat Nov 20, 2015

Member

This is assuming every line that contains { and that is not matched by the SCSS types selector above is a block of CSS rules.

I'm not sure this is really bulletproof, and would like to double check this. What about @keyframes rules for example? Or @media and nested rules?

That was probably one of the reasons we didn't want to allow to comment CSS classes. 😄

@valeriangalliat

valeriangalliat Nov 20, 2015

Member

This is assuming every line that contains { and that is not matched by the SCSS types selector above is a block of CSS rules.

I'm not sure this is really bulletproof, and would like to double check this. What about @keyframes rules for example? Or @media and nested rules?

That was probably one of the reasons we didn't want to allow to comment CSS classes. 😄

This comment has been minimized.

@carljm

carljm Nov 20, 2015

Contributor

Yes, you're right about the assumption this makes. But @keyframes and @media rules are still CSS rules, so they are not a counter-example to the assumption. Currently they can't be commented at all; now they can be commented like any other CSS rule, so nothing breaks, just new opportunities are available for those who want to play with them. Nested rules are handled fine (that is, they aren't really handled at all, they just become part of the body / code -- but that's the same as nested stuff inside a mixin or whatever).

You're right that this is a rather broad rule, but I can't think of any real case where it would break. And it only ever comes into play in situations where currently your comments are just thrown away silently, so again, it doesn't break any current use case, just adds the potential for new ones.

@carljm

carljm Nov 20, 2015

Contributor

Yes, you're right about the assumption this makes. But @keyframes and @media rules are still CSS rules, so they are not a counter-example to the assumption. Currently they can't be commented at all; now they can be commented like any other CSS rule, so nothing breaks, just new opportunities are available for those who want to play with them. Nested rules are handled fine (that is, they aren't really handled at all, they just become part of the body / code -- but that's the same as nested stuff inside a mixin or whatever).

You're right that this is a rather broad rule, but I can't think of any real case where it would break. And it only ever comes into play in situations where currently your comments are just thrown away silently, so again, it doesn't break any current use case, just adds the potential for new ones.

This comment has been minimized.

@valeriangalliat

valeriangalliat Nov 20, 2015

Member

it doesn't break any current use case, just adds the potential for new ones.

Sight. Sounds good.

@valeriangalliat

valeriangalliat Nov 20, 2015

Member

it doesn't break any current use case, just adds the potential for new ones.

Sight. Sounds good.

@pascalduez

This comment has been minimized.

Show comment
Hide comment
@pascalduez

pascalduez Nov 20, 2015

Member

Conceptually I'm in favor of this, I feel like it's time to let people be able to hack around SassDoc, so they can build custom themes that fits their needs, new directions, etc...
At the time of PostCSS, CSS modules, and all possible new concepts to come, it might be wise not to lock SassDoc evolution capabilities.
Which doesn't mean SassDoc core (default theme) have to change guidance, if we still want to focus on Sass libraries, we can.

I'll have a look at the code later on.

Member

pascalduez commented Nov 20, 2015

Conceptually I'm in favor of this, I feel like it's time to let people be able to hack around SassDoc, so they can build custom themes that fits their needs, new directions, etc...
At the time of PostCSS, CSS modules, and all possible new concepts to come, it might be wise not to lock SassDoc evolution capabilities.
Which doesn't mean SassDoc core (default theme) have to change guidance, if we still want to focus on Sass libraries, we can.

I'll have a look at the code later on.

@HugoGiraudel

This comment has been minimized.

Show comment
Hide comment
@HugoGiraudel

HugoGiraudel Nov 20, 2015

Member

Ohai.

Thanks again for bringing this on the table @carljm (and @ericam).

On the code side, it looks simple and clever enough to be merged without worrying too much, so I’d go with a yes. It looks like a nice addition that does not introduce any backward incompatibility, therefore I do not see any reason to prevent this or decline this pull request.

Now, on the documentation side I am wondering whether / how we should document this. I would be in favor of not promoting this too much if we want SassDoc to remain an API documentation tool. That being said, we could make this reply from the FAQ a bit more elaborate, perhaps including a link to this pull-request and any further work performed by the OddBird team.

On top of that, it would be interesting to document this somewhere else in the “Custom theme” section, so people know that they can use this feature if they build their own theme. Indeed, I must say I don’t really want to adapt the default theme to support CSS rules, although maybe we should. Thoughts?

Member

HugoGiraudel commented Nov 20, 2015

Ohai.

Thanks again for bringing this on the table @carljm (and @ericam).

On the code side, it looks simple and clever enough to be merged without worrying too much, so I’d go with a yes. It looks like a nice addition that does not introduce any backward incompatibility, therefore I do not see any reason to prevent this or decline this pull request.

Now, on the documentation side I am wondering whether / how we should document this. I would be in favor of not promoting this too much if we want SassDoc to remain an API documentation tool. That being said, we could make this reply from the FAQ a bit more elaborate, perhaps including a link to this pull-request and any further work performed by the OddBird team.

On top of that, it would be interesting to document this somewhere else in the “Custom theme” section, so people know that they can use this feature if they build their own theme. Indeed, I must say I don’t really want to adapt the default theme to support CSS rules, although maybe we should. Thoughts?

@carljm

This comment has been minimized.

Show comment
Hide comment
@carljm

carljm Nov 20, 2015

Contributor

Thanks for the review and discussion! I don't personally have any opinion on how or whether the SassDoc documentation, FAQ, or default theme should be updated, but if you all decide that they should be, I'm happy to submit a PR to SassDoc for that as well. Just let me know.

Regarding this PR, let me know if you want the keys updated from name and value to either name and code or selector and body. That's the only outstanding question I'm aware of.

Contributor

carljm commented Nov 20, 2015

Thanks for the review and discussion! I don't personally have any opinion on how or whether the SassDoc documentation, FAQ, or default theme should be updated, but if you all decide that they should be, I'm happy to submit a PR to SassDoc for that as well. Just let me know.

Regarding this PR, let me know if you want the keys updated from name and value to either name and code or selector and body. That's the only outstanding question I'm aware of.

@carljm

This comment has been minimized.

Show comment
Hide comment
@carljm

carljm Nov 25, 2015

Contributor

What's the next step here? Sounds like the consensus so far is in favor of merging -- any changes needed first? Anyone else need to weigh in?

Contributor

carljm commented Nov 25, 2015

What's the next step here? Sounds like the consensus so far is in favor of merging -- any changes needed first? Anyone else need to weigh in?

@HugoGiraudel

This comment has been minimized.

Show comment
Hide comment
@HugoGiraudel

HugoGiraudel Nov 25, 2015

Member

I would have loved to have @FWeinb but I don’t think it’s gonna happen. @pascalduez, @valeriangalliat, all good?

Member

HugoGiraudel commented Nov 25, 2015

I would have loved to have @FWeinb but I don’t think it’s gonna happen. @pascalduez, @valeriangalliat, all good?

@valeriangalliat

This comment has been minimized.

Show comment
Hide comment
@valeriangalliat

valeriangalliat Nov 27, 2015

Member

Looks good to me

Member

valeriangalliat commented Nov 27, 2015

Looks good to me

@pascalduez

This comment has been minimized.

Show comment
Hide comment
@pascalduez

pascalduez Nov 27, 2015

Member

👍

Member

pascalduez commented Nov 27, 2015

👍

@pascalduez

This comment has been minimized.

Show comment
Hide comment
@pascalduez

pascalduez Nov 28, 2015

Member

Let's see this as a shy hello sign to future evolutions and theme hackers.
But feels like we would need a serious planning at some point if we want to do things right.
Especially the data interface naming.

Member

pascalduez commented Nov 28, 2015

Let's see this as a shy hello sign to future evolutions and theme hackers.
But feels like we would need a serious planning at some point if we want to do things right.
Especially the data interface naming.

pascalduez added a commit that referenced this pull request Nov 28, 2015

Merge pull request #21 from oddbird/css-context
Can parse CSS rule code contexts, too.

@pascalduez pascalduez merged commit e0e6e71 into SassDoc:master Nov 28, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@pascalduez

This comment has been minimized.

Show comment
Hide comment
@pascalduez

pascalduez Nov 28, 2015

Member

Thanks for the PR @carljm !

Available as of scss-comment-parser@0.8.0 and sassdoc@2.1.16

Member

pascalduez commented Nov 28, 2015

Thanks for the PR @carljm !

Available as of scss-comment-parser@0.8.0 and sassdoc@2.1.16

@pascalduez pascalduez referenced this pull request in SassDoc/sassdoc.github.io Nov 28, 2015

Open

Document CSS context parsing #126

@carljm

This comment has been minimized.

Show comment
Hide comment
@carljm

carljm Nov 28, 2015

Contributor

Thanks @pascalduez !

Contributor

carljm commented Nov 28, 2015

Thanks @pascalduez !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment