Skip to content
This repository was archived by the owner on Sep 6, 2021. It is now read-only.

[Initial review] Update rule label when selector is edited#5541

Merged
dangoor merged 7 commits intomasterfrom
nj/update-rule-label
Oct 17, 2013
Merged

[Initial review] Update rule label when selector is edited#5541
dangoor merged 7 commits intomasterfrom
nj/update-rule-label

Conversation

@njx
Copy link
Copy Markdown

@njx njx commented Oct 17, 2013

@dangoor - note that this is branched off the css-quick-edit-new-selector branch from #5532 (I didn't want to disturb your review there). The commits in this one (607b35d, df1d141, 73e7414) are semantic so can be reviewed individually.

@njx
Copy link
Copy Markdown
Author

njx commented Oct 17, 2013

@peterflynn - might want to do a drive-by on df1d141

@njx
Copy link
Copy Markdown
Author

njx commented Oct 17, 2013

One thing that's a little skeezy about this is that the way we update the selector when you make an edit is different from how we collect the selectors originally. This is mainly because of the mismatch between what findMatchingRules does (which searches by individual selector) and what we want here, which is to get all the selectors for a rule. I felt that doing it a different way here was fine since this aspect of CSS syntax is pretty simple.

@njx
Copy link
Copy Markdown
Author

njx commented Oct 17, 2013

Hmmm...thinking about this again, I suppose this is a little more skeezy than I thought. For example, there could be a comment between the selector list and the beginning of the rule, and we would end up sucking that in. Doesn't seem super-likely, but it could happen...

It also occurs to me that another edge case would be if you had multiple rules on the same line--since TextRange only tracks line numbers, not offsets within a line, we would grab the first selector on the line even if the rule were later on the same line. Again, doesn't seem super-likely (aside from minified files, which would be a mess in an inline editor anyway)...but adding that level of tracking to TextRange is definitely more work (though perhaps not terrible).

Comments welcome...

@dangoor
Copy link
Copy Markdown
Contributor

dangoor commented Oct 17, 2013

The code all looks fine for what I does.

It would be good, I think, if the mechanism used for getting the updated selector was the same. I tried out the edge cases you pointed out, and they are a bit ugly. That said, these are edge cases and I don't know how likely it is that someone would hit them in real life. Do people really put multiple rules on the same line in source files?

If it was quick and easy to change this, then I think we're better off fixing these edge cases now. If not, I think we should wait and see if anyone complains because it's entirely possible that we'd never hear a complaint about this.

@ghost ghost assigned dangoor Oct 17, 2013
@njx
Copy link
Copy Markdown
Author

njx commented Oct 17, 2013

I think the only case for multiple lines on the same file is in a minified file. Other than that, I've never seen it.

It seems like there are basically two separate things we would want to fix:

(1) Use the real tokenizer to get the updated selector. I just realized that I could probably use extractAllSelectors() for this now since I just made it track selector groups. So I'll fix that.
(2) Make sure to get the right rule if there are multiple rules on the same line by extending TextRange to handle character offsets. I think this is the one that is lower bang-for-the-buck, since it's really unlikely that people will hit it. I'll see how much work it is, though--if it's not that bad I might as well just do it. (Seems like it might be useful in other cases too.)

@dangoor
Copy link
Copy Markdown
Contributor

dangoor commented Oct 17, 2013

That sounds like a good plan.

@njx
Copy link
Copy Markdown
Author

njx commented Oct 17, 2013

@dangoor - I pushed up the change to make getRangeSelectors() use the existing extractAllSelectors() code. I decided not to go for the TextRange change right now since it would be fairly intricate and doesn't seem worth the effort given the unlikelihood of the use case. So I think this is ready for review.

@dangoor
Copy link
Copy Markdown
Contributor

dangoor commented Oct 17, 2013

Looks good. Merging.

dangoor added a commit that referenced this pull request Oct 17, 2013
[Initial review] Update rule label when selector is edited
@dangoor dangoor merged commit 2d9166e into master Oct 17, 2013
@dangoor dangoor deleted the nj/update-rule-label branch October 17, 2013 20:46
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants