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

text-shadow should not accept negative value for its third length #15999

Closed
upsuper opened this issue Mar 17, 2017 · 10 comments
Closed

text-shadow should not accept negative value for its third length #15999

upsuper opened this issue Mar 17, 2017 · 10 comments

Comments

@upsuper
Copy link
Member

@upsuper upsuper commented Mar 17, 2017

I thought it should be fixed in #15611 but apparently it didn't.

It seems text-shadow doesn't actually use Shadow::parse, but it is probably better have it share the same parsing code.

If we want to keep the SpecifiedTextShadow struct rather than using Shadow (for saving memory?), it is probably better having a conversion function from Shadow to that, rather than having separate parsing code.

@karan1276, since you worked on #15490, would you like to have a look at this issue as well?

@karan1276
Copy link
Contributor

@karan1276 karan1276 commented Mar 17, 2017

@upsuper sure, why not.

@upsuper
Copy link
Member Author

@upsuper upsuper commented Mar 17, 2017

Thanks.

@upsuper upsuper added the C-assigned label Mar 17, 2017
@bholley bholley added the A-stylo label Mar 26, 2017
@highfive
Copy link

@highfive highfive commented Mar 26, 2017

cc @emilio

@karan1276
Copy link
Contributor

@karan1276 karan1276 commented Mar 27, 2017

I am going out of town for a week. Once i get back i will start work on this

@jdm jdm removed the C-assigned label Mar 27, 2017
@karan1276
Copy link
Contributor

@karan1276 karan1276 commented Apr 4, 2017

I was looking at the code. I have a few questions. Didn't we implement Shadow so properties like this can call it? or is there another use for it.
About the conversion system, how exactly should i go about it?

The structure of SpecifiedTextShadow and Shadow is almost same. Wouldn't it be better to use the Shadow object and call the Shadow::parse method (just guessing) ?

@karan1276
Copy link
Contributor

@karan1276 karan1276 commented Apr 4, 2017

@highfive: assign me

@highfive highfive added the C-assigned label Apr 4, 2017
@highfive
Copy link

@highfive highfive commented Apr 4, 2017

Hey @karan1276! Thanks for your interest in working on this issue. It's now assigned to you!

@upsuper
Copy link
Member Author

@upsuper upsuper commented Apr 9, 2017

The structure of SpecifiedTextShadow and Shadow is almost same. Wouldn't it be better to use the Shadow object and call the Shadow::parse method (just guessing) ?

That was my suggestion at the top of the issue as well.

@karan1276
Copy link
Contributor

@karan1276 karan1276 commented Apr 12, 2017

okay ill make changes and send pr. Ill probably get it wrong the first time, but lets see

@upsuper
Copy link
Member Author

@upsuper upsuper commented May 24, 2017

This seem to have been fixed.

@upsuper upsuper closed this May 24, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
5 participants
You can’t perform that action at this time.