-
Notifications
You must be signed in to change notification settings - Fork 342
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
refactor!(ui, llc): Attachments refactor. #1667
Conversation
ce90e0f
to
4699e65
Compare
9750ddd
to
bd1bf0a
Compare
Codecov ReportPatch coverage:
❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more. Additional details and impacted files@@ Coverage Diff @@
## v7.0.0 #1667 +/- ##
==========================================
+ Coverage 58.47% 58.55% +0.07%
==========================================
Files 290 310 +20
Lines 17652 17772 +120
==========================================
+ Hits 10322 10406 +84
- Misses 7330 7366 +36
☔ View full report in Codecov by Sentry. |
Signed-off-by: xsahil03x <[email protected]>
Signed-off-by: xsahil03x <[email protected]>
Signed-off-by: xsahil03x <[email protected]>
Signed-off-by: xsahil03x <[email protected]>
Signed-off-by: xsahil03x <[email protected]>
Signed-off-by: xsahil03x <[email protected]>
Signed-off-by: xsahil03x <[email protected]>
318b0b3
to
72e0c7b
Compare
Signed-off-by: xsahil03x <[email protected]>
Signed-off-by: xsahil03x <[email protected]>
Signed-off-by: xsahil03x <[email protected]>
// If the attachment contains titleLink but is not of type giphy, we | ||
// consider it as a urlPreview. |
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.
This comment just duplicates the code below.
Could we put the reason why we're doing this instead (why we consider it as urlPreview
)? 🙂
MediaType? get mimeType => name?.mimeType; | ||
MediaType? get mediaType => name?.mediaType; |
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.
Is this a breaking change?
Should we do the deprecation cycle before replacing it with mediaType
?
/// {@template giphy_info} | ||
/// A class that contains extra information about a Giphy attachment. | ||
/// {@endtemplate} | ||
class GiphyInfo { |
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.
Just curious if need Equatable
here.
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 needed. Used just to store some values.
extension GiphyInfoX on Attachment { | ||
/// Returns the [GiphyInfo] for the given [type]. | ||
GiphyInfo? giphyInfo(GiphyInfoType type) { | ||
final giphy = extraData['giphy'] as Map<String, Object?>?; |
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.
Could we extract 'giphy'
as shared const or enum?
Is this the only place where we use hardcoded 'giphy'
value?
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.
Is this a breaking change?
It looks like this was an exported class.
@@ -256,7 +251,8 @@ class MessageInputMediaAttachments extends StatelessWidget { | |||
height: 104, | |||
child: ListView( | |||
scrollDirection: Axis.horizontal, | |||
padding: const EdgeInsets.symmetric(horizontal: 8), | |||
padding: const EdgeInsets.symmetric(vertical: 2, horizontal: 8), | |||
cacheExtent: 104 * 10, // Cache 10 items ahead. |
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.
Curious where the magic number 104
comes from
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.
figma design
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.
Is it a breaking change?
// attachmentBuilders = { | ||
// // Add all default builders | ||
// 'image': (context, message, attachments) { | ||
// final color = StreamChatTheme.of(context).colorTheme.borders; | ||
// final border = RoundedRectangleBorder( | ||
// side: attachmentBorderSide ?? BorderSide(color: color), | ||
// borderRadius: attachmentBorderRadiusGeometry ?? BorderRadius.zero, | ||
// ); | ||
// | ||
// if (attachments.length > 1) { | ||
// return WrapAttachmentWidget( | ||
// attachmentShape: border, | ||
// attachmentWidget: Material( |
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.
Do we still need this commented out section?
this.attachmentShape, | ||
this.borderSide, | ||
this.attachmentBorderSide, | ||
this.borderRadiusGeometry, | ||
this.attachmentBorderRadiusGeometry, | ||
this.attachmentShape, |
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.
Possible breaking change?
required this.video, | ||
this.headers, | ||
this.imageFormat = ImageFormat.PNG, | ||
this.maxHeight = 0, | ||
this.maxWidth = 0, | ||
this.timeMs = 0, | ||
this.quality = 10, | ||
this.scale = 1.0, |
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.
Seems like a breaking change as well.
@kanat yes, this PR will be part of V7 of the SDK. We need to write migration guides and docs along with it. |
# Conflicts: # packages/stream_chat_flutter/lib/src/fullscreen_media/full_screen_media.dart # packages/stream_chat_flutter/lib/src/fullscreen_media/full_screen_media_desktop.dart # packages/stream_chat_flutter/pubspec.yaml
Signed-off-by: Sahil Kumar <[email protected]>
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.
LGTM
Fixes: #1621, #1545
Screen.Recording.2023-07-11.at.3.59.26.PM.mov
New.Attachments.mov