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.

Sign up for GitHub

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

Jump to bottom

use CSS instead of inlining styles 🖌️ #181

Merged
merged 1 commit into from
May 15, 2024
Merged

use CSS instead of inlining styles 🖌️ #181

merged 1 commit into from
May 15, 2024

Conversation

jamacku
Copy link
Member

@jamacku jamacku commented May 10, 2024

The branch contains additional commits from:

Once they are accepted, I'll drop them.

This is just a DRAFT. There is probably a better way how to implement CSS into writer-html

@jamacku
Copy link
Member Author

jamacku commented May 10, 2024

Related to #174 ?

ferdnyc
ferdnyc reviewed May 11, 2024
View reviewed changes
Copy link

@ferdnyc ferdnyc left a comment

Choose a reason for hiding this comment

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

This definitely looks like a HUGE step in the right direction, @jamacku. Thanks for looking at it!

src/lib/writer-html.cc Outdated Show resolved Hide resolved
kdudka
kdudka reviewed May 13, 2024
View reviewed changes
Copy link
Member

@kdudka kdudka left a comment

Choose a reason for hiding this comment

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

@jamacku Looks good! A few suggestions inline...

src/lib/writer-html.cc Outdated Show resolved Hide resolved
src/lib/writer-html.cc Outdated Show resolved Hide resolved
src/lib/writer-html.cc Outdated Show resolved Hide resolved
src/lib/writer-html.cc Outdated Show resolved Hide resolved
jamacku added a commit to jamacku/csdiff that referenced this pull request May 14, 2024
@jamacku
feat(cshtml): use CSS instead of inlining styles
5f3b933 
Fixes: csutils#174
Closes: csutils#181
@jamacku jamacku self-assigned this May 14, 2024
@jamacku
Copy link
Member Author

jamacku commented May 14, 2024

@kdudka, @ferdnyc, thanks for the review. I have addressed your comments in 5f3b933.

@jamacku jamacku marked this pull request as ready for review May 14, 2024 08:37
@jamacku jamacku requested a review from kdudka May 14, 2024 08:39
lzaoral
lzaoral reviewed May 14, 2024
View reviewed changes
src/lib/writer-html.cc Outdated Show resolved Hide resolved
@jamacku jamacku requested a review from lzaoral May 14, 2024 12:48
kdudka
kdudka reviewed May 14, 2024
View reviewed changes
Copy link
Member

@kdudka kdudka left a comment

Choose a reason for hiding this comment

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

@jamacku Thanks for the update! Two more remarks inline...

src/lib/writer-html.cc Outdated Show resolved Hide resolved
src/lib/writer-html.cc Outdated Show resolved Hide resolved
@jamacku jamacku requested a review from kdudka May 15, 2024 07:08
kdudka
kdudka approved these changes May 15, 2024
View reviewed changes
Copy link
Member

@kdudka kdudka left a comment

Choose a reason for hiding this comment

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

@jamacku Looks good. Thanks for the update!

@jamacku jamacku merged commit a8f67f1 into csutils:main May 15, 2024
33 checks passed
@jamacku jamacku deleted the css branch May 15, 2024 10:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ferdnyc ferdnyc ferdnyc left review comments

@kdudka kdudka kdudka approved these changes

@lzaoral lzaoral lzaoral approved these changes

Assignees

@jamacku jamacku

Labels
enhancement html
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@jamacku @kdudka @ferdnyc @lzaoral