Make newlines visible in deleted messages#304
Conversation
#302 This commit makes newlines in deleted messages visible in the deleted messages front-end and makes sure they are not stripped during the conversion to HTML. To represent newlines, I've chosen a commonly used symbol: `↵`. In addition, I've kaizened the colour filter that translates integer representations of colours to their RGB hex-value. The Discord dark theme shows black colours (int: 0; hex: #000000) as white instead, to make reading them against the dark background easier. This commit makes sure our front-end displays the same behavior. This closes #302
SebastiaanZ
added
enhancement
area: frontend
There was a problem hiding this comment.
I definitely approve of the unit test output changes, particularly with organizing them into subtests, and iterating over a tuple of actual and expected results instead of a wall of self.assertEqual(). Here's a couple of minor suggested improvements along the same lines (making the tests more readable and easier to debug), feel free to rename the vars or reformat:
| for colour, hex_value in test_values: | ||
| with self.subTest(colour=colour, hex_value=hex_value): | ||
| self.assertEqual(deletedmessage_filters.hex_colour(colour), hex_value) |
There was a problem hiding this comment.
for colour, hex_value in test_values:
actual_hex = deletedmessage_filters.hex_colour(colour)
with self.subTest(colour=colour, actual_hex=actual_hex,
expected_hex=hex_value):
self.assertEqual(actual_hex, expected_hex)This has no functional difference, but it would make it easier to tell at a glance what the issue was if/when the tests fail. I find that making use of the var names also helps to self-document the exact purpose of the tests.
There was a problem hiding this comment.
Wouldn't the AssertionError message for an assertEqual test already contain both values?
| for input_, expected_output in test_values: | ||
| with self.subTest(input=input_, expected_output=expected_output): | ||
| self.assertEqual(deletedmessage_filters.visible_newlines(input_), expected_output) |
There was a problem hiding this comment.
for text, expected_output in test_values:
actual_output = deletedmessage_filters.visible_newlines(text)
with self.subTest(text=text, actual_output=actual_output,
expected_output=expected_output):
self.assertEqual(actual_output, expected_output)
MarkKoz
added
type: feature
jb3
requested review from
a team,
Inveracity,
ikuyarihS and
jb3
and removed request for
a team and
Inveracity
lemonsaurus
left a comment
lemonsaurus
left a comment
There was a problem hiding this comment.
I think we should get this merged.
7 participants
This pull requests makes the newlines in deleted messages visible using the
↵symbol. This should make it easier to correctly judge the deleted messages that show up in our deleted messages front-end.Screenshot
In addition, I've kaizened the colour filter that translates integer representations of colours to their RGB hex-value. The Discord dark theme shows black colours (int: 0; hex: #000000) as white (#FFFFFF) to make reading text having that color easier against the dark background. This pull request makes sure our front-end displays the same behavior.
This pull request closes #302