We had a bug in our thread settings where we would show ui elements for the media gallery even if no items were present. This diff fixes that
Linear task: https://linear.app/comm/issue/ENG-3301/[native]-empty-media-gallery-looks-out-of-place
Details
Details
Please see the demo video below
Diff Detail
Diff Detail
- Repository
- rCOMM Comm
- Lint
No Lint Coverage - Unit
No Test Coverage
Event Timeline
| native/chat/settings/thread-settings-media-gallery.react.js | ||
|---|---|---|
| 34 | This is optional because we use this component in other places where we don't want the "See more" button. | |
Comment Actions
Taking over the review here to try and unblock this so @ginsu can finish up his work.
Merging header/footer
Normally we'd avoid merging the header and footer into ThreadSettingsMediaGallery. We intentionally keep these separated as their own items in the list to improve rendering performance. Virtualized lists work best when their items are as short as possible.
That said, since ThreadSettings doesn't have easy access to the list of media, and since the media is fetched on-demand, I think it's more trouble than its worth. So let's leave the diff as-is in this respect.
Footer was removed?
However, I do have one concern about the changes here: you appear to have removed the footer wholesale. Let's avoid regressing functionality, or at the very least justify why we're completely removing the footer, and include a before/after screenshot if the UI looks any different.
| native/chat/settings/thread-settings-media-gallery.react.js | ||
|---|---|---|
| 37 ↗ | (On Diff #42090) | This is optional because we use this component in other places where we don't want the "See more" button. |