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

Feature/#19 inline styles #305

Merged
merged 5 commits into from
Nov 22, 2020
Merged

Feature/#19 inline styles #305

merged 5 commits into from
Nov 22, 2020

Conversation

vanlooverenkoen
Copy link
Contributor

@vanlooverenkoen vanlooverenkoen commented May 27, 2020

#19 add support for inline colors.

@Sub6Resources I used your inline styles branch for finishing the color inline styles. as well as the background-color property. I think we can use this and incrementally add new css attributes

@vanlooverenkoen
Copy link
Contributor Author

@ryan-berger @Sub6Resources @edwardaux any idea when this pr will be reviewed/merged

@ryan-berger
Copy link
Collaborator

@vanlooverenkoen Wow, this is great. Thank you for the implementation.

I'm going to need to take a bit on this one. I'll have to take some time to walk through all of this because it is a huge feature with some complicated code, and I want to give it due diligence. This may not be merged for another month depending on how busy my school gets, but if you want to try and get others to help review, it would be extremely helpful, and would lighten the load a bit.

No matter what, I will still have to walk through it, but if there are some good comments before I start chipping away, it could be helpful.

@vanlooverenkoen
Copy link
Contributor Author

@ryan-berger any idea when this could be merged? We are using my fork in production for over 2 months

@ryan-berger
Copy link
Collaborator

I'd like to merge this in, but again, it hasn't quite been a month, and I have a good amount of school work along with lots of contract work that I have on my plate. In order to give this its due diligence I have to be able to have some downtime. Next weekend will likely be a good time for reviewing this, but I can't make any guarantees as I do system administration and we've recently had many incidents that have taken most of my time.

It is nice to know that you have been using this in production as that eases up on the amount of testing that I'll likely need to do, but whether or not it is an implementation that I think effectively implements inline styles into flutter_html is a different issue entirely, and I can't make any judgements about that until I read through it and see how it integrates itself with the library.

Again, if you can get more people to review this, I can feel more comfortable merging this with less due diligence.

@vanlooverenkoen
Copy link
Contributor Author

I for us it is less important to have it merged right away. We can still use our fork. But I can image other users want to have this as well. I was just following up on some pull requests I created last month. Thanks in advance

Copy link
Collaborator

@erickok erickok left a comment

Choose a reason for hiding this comment

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

This is excellent work. I tested this in a bunch of variations and proved reliable. Thanks! Good to merge for me.

@vanlooverenkoen
Copy link
Contributor Author

Yes again we are still using my fork for our production app. No bugs after 4 months of use.

@ryan-berger
Copy link
Collaborator

If @erickok has used this and production has been working well, I'll merge this in. I haven't been able to test it read too hard through the code, but my winter break is coming up here soon so I should have much more time to look into this, and if we need to edit this later, we can do so.

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.

3 participants