Skip to content

Conversation

sirpengi
Copy link
Contributor

@sirpengi sirpengi commented Feb 5, 2024

Fixes: #354

Screenshot_20240205_162052

Copy link
Member

@gnprice gnprice left a comment

Choose a reason for hiding this comment

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

Thanks @sirpengi! Generally this looks good; just small comments below.

}

if (localName == 'time' && classes.isEmpty) {
final attr = element.attributes['datetime'];
Copy link
Member

Choose a reason for hiding this comment

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

nit:

Suggested change
final attr = element.attributes['datetime'];
final datetimeAttr = element.attributes['datetime'];

Comment on lines 747 to 752
final DateTime datetime;
try {
datetime = DateTime.parse(attr);
} on FormatException {
return unimplemented();
}
Copy link
Member

Choose a reason for hiding this comment

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

This can be cleaned up a bit using the tryParse method:

Suggested change
final DateTime datetime;
try {
datetime = DateTime.parse(attr);
} on FormatException {
return unimplemented();
}
final datetime = DateTime.tryParse(attr);
if (datetime == null) return unimplemented();


group('global times', () {
testParseInline('smoke',
// "<time:2024-01-30T17:33:00Z">"
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// "<time:2024-01-30T17:33:00Z">"
// "<time:2024-01-30T17:33:00Z>"

right?

// borderRadius: BorderRadius.all(Radius.circular(3))));
}

class GlobalTime extends StatelessWidget {
Copy link
Member

Choose a reason for hiding this comment

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

nit: put this after MessageImageEmoji, to match the if/else-if chain above (which matches the node-class definitions)

tester.widget(find.text(r'\lambda'));
});

testWidgets('GlobalTime smoke', (tester) async {
Copy link
Member

Choose a reason for hiding this comment

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

nit: put this after the MathInlineNode test below, to match the code under test in lib/widgets/content.dart

@sirpengi
Copy link
Contributor Author

sirpengi commented Feb 6, 2024

@gnprice ready for review again!

Sourced today from:
  https://www.figma.com/file/1JTNtYo9memgW7vV6d0ygq/Zulip-Mobile?node-id=164%3A10509&mode=dev

Output was processed using inkscape to expand the strokes to
paths and clean up unnecessary clipping.

Also refer to discussion in CZO:
  https://chat.zulip.org/#narrow/stream/101-design/topic/clock.20icons/near/1728373
@gnprice gnprice merged commit e1bff3e into zulip:main Feb 7, 2024
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.

Handle <time> elements ("global times")

2 participants