-
Notifications
You must be signed in to change notification settings - Fork 0
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
Add colours to dels and ins #16
Add colours to dels and ins #16
Conversation
src/Forms/DiffTransformation.php
Outdated
@@ -110,7 +110,7 @@ public function transform(FormField $field) | |||
Diff::compareHTML($field->Value(), isset($this->data[$name]) ? $this->data[$name] : '') | |||
); | |||
|
|||
$diffField->addExtraClass($field->extraClass()); | |||
$diffField->addExtraClass($field->extraClass() . "history-viewer__version-detail-diff"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add a space at the start of the quoted string, or add it with a separate call to addExtraClass
border-radius: .192 rem; | ||
padding: .192 rem; | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you please run this through a linter?
background-color: $green; | ||
color: white; | ||
border-radius: .192 rem; | ||
padding: .192 rem; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not sure if the linter is picking this up, but you shouldn't have spaces between the number and the unit, also please remove the empty line on 175
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Neither yarn lint
nor yarn build
did... But I have refactored that code anyway in the latest commit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Beautiful. My only concern is that I think these styles should be added to the admin module in case other devs want to display diffs in react forms without this module, however they’d also need the DiffTransformation, and we could/should move it over later on
Asked @clarkepaul for some further instructions, but haven't received an answer yet.