Skip to content
This repository has been archived by the owner on Sep 11, 2024. It is now read-only.

Improve handling of the disabled state of the save button in the edit message composer #8601

Closed
wants to merge 11 commits into from

Conversation

SimonBrandner
Copy link
Contributor

@SimonBrandner SimonBrandner commented May 15, 2022

Fixes element-hq/element-web#22207


Peek.2022-05-15.09-41.mp4

Here's what your changelog entry will look like:

✨ Features

@SimonBrandner SimonBrandner added the T-Enhancement New features, changes in functionality, performance boosts, user-facing improvements label May 15, 2022
@SimonBrandner SimonBrandner requested a review from a team as a code owner May 15, 2022 07:42
@SimonBrandner SimonBrandner changed the title Improve handling of the disabled state of the save button while editing Improve handling of the disabled state of the save button in the edit message composer May 15, 2022
@SimonBrandner SimonBrandner force-pushed the SimonBrandner/feat/improved-edits branch 2 times, most recently from d869c67 to 3a5e4c0 Compare May 15, 2022 07:45
Signed-off-by: Šimon Brandner <simon.bra.ag@gmail.com>
Signed-off-by: Šimon Brandner <simon.bra.ag@gmail.com>
Signed-off-by: Šimon Brandner <simon.bra.ag@gmail.com>
Signed-off-by: Šimon Brandner <simon.bra.ag@gmail.com>
Signed-off-by: Šimon Brandner <simon.bra.ag@gmail.com>
Signed-off-by: Šimon Brandner <simon.bra.ag@gmail.com>
Signed-off-by: Šimon Brandner <simon.bra.ag@gmail.com>
Signed-off-by: Šimon Brandner <simon.bra.ag@gmail.com>
Signed-off-by: Šimon Brandner <simon.bra.ag@gmail.com>
Signed-off-by: Šimon Brandner <simon.bra.ag@gmail.com>
@t3chguy
Copy link
Member

t3chguy commented May 16, 2022

This was done intentionally to begin with, the Save button felt weird being enabled & re-disabled, so the Save action itself actually skipped sending the event edit if there was no change in the end

Copy link
Member

@turt2live turt2live left a comment

Choose a reason for hiding this comment

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

this is a surprising amount of code change for a relatively mundane feature - will wait for design to provide feedback before reviewing this.

@HarHarLinks
Copy link
Contributor

(I raised this issue)

This was done intentionally to begin with, the Save button felt weird being enabled & re-disabled, so the Save action itself actually skipped sending the event edit if there was no change in the end

This behaviour is fine by me, I would even say it makes sense. However the part where the save action skips sending should also apply to keyboard navigation, i.e. should prohibit dismissing via up/down arrows only when the message is actually edited.

@amshakal amshakal requested review from niquewoodhouse and removed request for a team May 31, 2022 16:17
@andybalaam
Copy link
Contributor

(I raised this issue)

This was done intentionally to begin with, the Save button felt weird being enabled & re-disabled, so the Save action itself actually skipped sending the event edit if there was no change in the end

This behaviour is fine by me, I would even say it makes sense. However the part where the save action skips sending should also apply to keyboard navigation, i.e. should prohibit dismissing via up/down arrows only when the message is actually edited.

See my message on the issue about why I closed it. I also want to address this comment.

I think if I am navigating with up/down and then edit a message, and then undo, I don't want up/down to move me away, for the same reasons as I gave in the issue: it adds a mental burden on me to understand the different behaviour, just because my edits ended up having no effect.

@andybalaam
Copy link
Contributor

Closing this PR since I close the related issue.

Thank you for your work on this!

@andybalaam andybalaam closed this Nov 2, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
T-Enhancement New features, changes in functionality, performance boosts, user-facing improvements
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Broken state when editing
5 participants