Add Foldable Images #4349
Add Foldable Images #4349
Conversation
|
Could you please post a gif of it in action? Thanks |
| const chevronClasses = classNames({ | ||
| 'mx_RoomSubList_chevron': true, | ||
| 'mx_RoomSubList_chevronRight': collapsed, | ||
| 'mx_RoomSubList_chevronLeft': !collapsed, |
There was a problem hiding this comment.
It would be better to split out the chevron to a common element if it's being used elsewhere, rather than using classes from a different component.
|
|
| @@ -122,6 +128,10 @@ export default class MImageBody extends React.Component { | |||
| ); | |||
| } | |||
|
|
|||
| _toggleCollapsed() { | |||
| this.setState({ collapsed: !this.state.collapsed }); | |||
There was a problem hiding this comment.
We'll need to call this.props.onHeightChanged() after this state change has rendered (comparing collapsed between prevState and current state in componentDidUpdate), to let the timeline code know this tile changed height. We do this to prevent potential scroll jumps afterwards.
|
As visible in the GIF, BACAT makes this behave a bit unexpected, in that older messages on top will take the place of the image, rather than newer messages from the bottom as one might expect. If the image is only partially in view at the bottom, this might (could you test?) completely hide the collapsed image tile below the viewport, which can be a bit disorienting. Not sure if this is a blocker necessarily, but wanted to mention. Fixing this could be a bit involved, but could be along the lines of passing an amount you want to scroll up in onHeightChanged, and implement support for that in |
|
I've added naive support for offsetting the scroll when onHeightChanged is called. I think I'd need a few minute conversation to understand ScrollPanel.js a little better before I'd be comfortable implementing a more proper solution in that module.
|
|
Hey @thedanisaur thanks for working on this. As it stands, we wouldn't merge this to master based on the current design, as the chevron implementation seems to always add extra unwanted vertical height to the timeline and lacks discoverability and affordance. I've added this to our Design Backlog here for us to take a closer look at and figure out where to go from there! |
OK, I look forward to seeing this in a future release.
|
…b.com:matrix-org/matrix-react-sdk into t3chguy/emoji_picker_composer � Conflicts: � src/components/views/rooms/MessageComposer.js
Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>
Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>
…b.com:matrix-org/matrix-react-sdk into t3chguy/emoji_picker_composer
…er_composer Rewire the Sticker button to be an Emoji Picker to release
Signed-off-by: Michael Telatynski <7t3chguy@gmail.com> (cherry picked from commit f4f23db)
`toastKey` is a thing. Fixes element-hq/element-web#13923 (cherry picked from commit c667ea6)
Fix compact layout regression
Fix not being able to dismiss new login toasts
renderTooltip was not a bound function and so was failing to find the parent when called from the 'scroll' event listener because 'this' was the window object rather than the Tooltip object. Unsure at what point this broke - I assumed it was in thr recent typescript conversion but it looks like it would have had the same problem before. (cherry picked from commit 5e569d7)
Fix exceptions from Tooltip
…dk into release-v2.7.1
|
@nadonomy the link you posted for the item in the backlog to reflect this idea doesn't appear to be public... is this still something that's in progress? This request has come up a few times on my team, and I'm contemplating whether to merge this code into my customized deployment or just wait for the feature to show up upstream. |
turt2live
requested review from
turt2live
and removed request for
a team and
janogarcia
|
Hey @thedanisaur - thanks for the contribution, and apologies for the massive delay in getting back to you on this. We've taken a fresh look and it appears as though the core concerns are addressed through other PRs. Namely, our images are now smaller in the timeline by default and there are options to hide images by default. As such, we've decided to close this PR for the time being. If you or someone else would like to see this feature still, we kindly ask that a fresh issue be opened on the element-web repo for the design/product teams to consider, now that in the modern age we can get in contact with them a lot faster ;) It also looks like your PR went sideways at some point, including changes from Thanks again for the contribution :) |
Where are those options located for element-web? In appearance settings, I only see this:
|
|
For further questions, #element-web:matrix.org is best. |
I have gone ahead and opened a request for this feature since I would still very much like to see this. It addresses the solutions presented here as alternatives. |
Fulfills feature request element-hq/element-web#2466: collapsible images.
Signed-off-by: Daniel DeFinis definisdan@yahoo.com