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

Resolve multiple sibling pseudo classes correctly #142

Merged
merged 2 commits into from
Jul 20, 2016

Conversation

tolgap
Copy link
Collaborator

@tolgap tolgap commented Jul 20, 2016

This PR aims to fix issue #136 .

The issue was: when resolving multiple sibling pseudo classes like:

[ a 
    [ color (hex "#000")
    , hover
        [ color (hex "#111") ]
    , active
        [ color (hex "#222") ]
    , disabled
        [ color (hex "#333") ]
    ]
]

the code written would resolve a, and next up, it would resolve a:hover and since :hover is a complete new styleblock definition, it got appended to the list of styleblock declarations. What happens next is that :active would get resolved, but since the latest styleblock is now an a:hover, it would think it's a nested pseudo class.

I have fixed this issue, by popping the last declaration, and using that to resolve pseudo classes. And in the end, using List.tail to get rid of the a declaration. This seems to fix my issues, and the issue shows in #136 as you can see in the written tests for this.

@rtfeldman since this issue was assigned to you, did you have any other concerns about this?

@tolgap
Copy link
Collaborator Author

tolgap commented Jul 20, 2016

The tests seem to be failing because of something in test-runner's elm-package.json.

@tolgap
Copy link
Collaborator Author

tolgap commented Jul 20, 2016

I will paste the fixtures and it's output here for ease of reading:

pseudo classes:

[ (#) Page
    [ color (hex "#fff")
    , hover
        [ marginTop (px 10)
        , focus
            [ color (hex "#000") ]
        ]
    , first
        [ fontSize (em 3) ]
    , disabled
        [ marginTop (px 20) ]
    ]
]
#Page {
    color: #fff;
}

#Page:hover {
    margin-top: 10px;
}

#Page:hover:focus {
    color: #000;
}

#Page:first {
    font-size: 3em;
}

#Page:disabled {
    margin-top: 20px;
}

@rtfeldman
Copy link
Owner

@tolgap wow nice! I just upgraded the test dependencies on master, which should resolve the test failure issue - would you mind rebasing on master?

@tolgap
Copy link
Collaborator Author

tolgap commented Jul 20, 2016

@rtfeldman rebased on master

@rtfeldman
Copy link
Owner

😻 😻 😻 This looks fantastic! Really appreciate it @tolgap!

@rtfeldman rtfeldman merged commit 93e56a5 into rtfeldman:master Jul 20, 2016
@rtfeldman
Copy link
Owner

rtfeldman commented Jul 20, 2016

@tolgap it looks like the second bug reported in #136 is still at large.

I reproduced it with a test case in b2a1968 (on branch reproduce-bug) if you have bandwidth to take a look. Thought I'd mention it since you did such an awesome job with this! ❤️

@tolgap
Copy link
Collaborator Author

tolgap commented Jul 20, 2016

@rtfeldman Forgot to include the explanation for that in my initial post...

I indeed could not resolve the second bug in #136 . That second issue is... quite hard to solve if I dare say 😨 . I do have some more time right now to look further into the second bug.

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

Successfully merging this pull request may close these issues.

2 participants