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

Bugfix/issue#3105 selector max id nested at statements #3113

Conversation

BenjaminWFox
Copy link
Contributor

Which issue, if any, is this issue related to?

#3105

Is there anything in the PR that needs further explanation?

I haven't done a lot of these (edits/PRs) so I'm not sure if I missed something, but I couldn't find a scenario in the tests where the the check for 'atrule' was useful. Everything seemed to work as I expected when I removed it.

Copy link
Member

@hudochenkov hudochenkov left a comment

Choose a reason for hiding this comment

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

Few minor organizational moments.

Could you rebase your branch on master, please? We found that some tests for selector-max-id weren't running and your fix might affect these tests because now they are running.

{
code: ".foo { @if ($p == 1) { #bar #baz {} } }",
description: "@if statement: nested compound selector"
},
Copy link
Member

Choose a reason for hiding this comment

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

I think tests containing @if should go to test function with syntax: "scss".

message: messages.expected(".foo #bar #baz #foo", 2),
line: 1,
column: 24
},
Copy link
Member

Choose a reason for hiding this comment

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

I think tests containing @if should go to test function with syntax: "scss".

@Arcanemagus
Copy link
Contributor

Arcanemagus commented Jan 11, 2018

@BenjaminWFox You should either merge changes from master into this branch, or rebase the branch on the new changes. Cherry picking the commits from master into here just causes confusion in the log when this is merged unless all of these commits are squashed together.

@BenjaminWFox
Copy link
Contributor Author

@Arcanemagus Hmm, I thought I rebased but I haven't dealt with upstream/forked repos before - I probably got confused trying to get the right updates from the right place.

Should I just squash them then? And would that be with something like git rebase -i <after-specific-commit>? Thanks.

@Arcanemagus
Copy link
Contributor

Actually as @hudochenkov just pointed out to me in a different PR, all PR's here are squashed so it's not a huge issue.

For your own reference though:

The simplest method is to make sure your local master matches the upstream master, then run git rebase master. That makes your commit(s) move to having the current master as their parent commit.

Alternatively you can also do git merge master, which creates a separate commit that merges the current state of master into your branch, leaving your existing commits untouched.

@ntwb ntwb dismissed hudochenkov’s stale review January 20, 2018 01:59

Requested changes were made in 803c2f0

Copy link
Member

@ntwb ntwb left a comment

Choose a reason for hiding this comment

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

LGTM 👍

• Tests pass locally for me ✅
• Squashing merge should work 🤞

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

Successfully merging this pull request may close these issues.

5 participants