Improvements to alt aliases #6180
Conversation
This should have some nicer behavior that will confused users less See element-hq/element-web#13501
Signed-off-by: Aaron Raimist <aaron@raim.ist>
|
Could we get screenshots/gif of the changes? |
|
When I get back to the computer I’ll take some but it looks exactly like the original PR. It’s best experienced by using the Netlify preview or building it yourself. |
Screen.Recording.2021-06-15.at.17.19.05.mov |
turt2live
added
the
X-Blocked
turt2live
removed
the
X-Blocked
There was a problem hiding this comment.
Thank you for working on bringing this back. I realize that the stuff I'm commenting on is not necessarily your code, but I do still have concerns with the approach given it introduces a moderately complex state machine for a component which doesn't appear to need it.
I haven't personally tested this, but from the looks of it the majority of the usability concerns were worked out already.
Design will probably want to take a look at this as well, as a sort of stopgap approach (hopefully). The spaces alias changes will almost certainly have collided with this too.
If it comes down to it, what is your interest in maintaining this and potentially pushing it forward with design support? Including potentially (re)writing a bunch of the state machine stuff.
| verifyRemove: boolean, | ||
| } | ||
|
|
||
| export class EditableItem extends React.Component<IEditableItemProps, IEditableItemState> { |
There was a problem hiding this comment.
I believe this class got converted in another PR (at least, I remember seeing the name today...). Should be able to use that safely, I hope.
| limitations under the License. | ||
| */ | ||
|
|
||
| import React, {useEffect, useRef, useState, FunctionComponent} from "react"; |
There was a problem hiding this comment.
applies throughout, as a new requirement: any non-component-property object definition should be padded with spaces. Eg: { words }
| interface AUpdateAlias { | ||
| type: 'update_alt_alias', | ||
| value: string, | ||
| } | ||
|
|
||
| interface ASyncedAliases { | ||
| type: 'synced_aliases', | ||
| alt: Alts, | ||
| canonical: string | undefined, | ||
| } | ||
|
|
||
| interface ABeginCommit { | ||
| type: 'begin_commit', | ||
| submitting: 'add' | 'remove' | 'canonical', | ||
| alt?: Alts, | ||
| canonical?: string, | ||
| regarding?: string, | ||
| } | ||
|
|
||
| interface AEndCommit { | ||
| type: 'end_commit' | ||
| } | ||
|
|
||
| interface AAbortCommit { | ||
| type: 'abort_commit', | ||
| error: Error | ||
| } | ||
|
|
||
| type Action = AUpdateAlias | ASyncedAliases | ABeginCommit | AEndCommit | AAbortCommit; |
There was a problem hiding this comment.
I'm a bit worried that this is a lot of extra complexity... Can we get comments as to what these are trying to achieve, and what problems they solve?
Also, our interfaces are INamed, so in this case would be IUpdateAliasAction and such. The type Action should probably be named something that doesn't conflict with the dispatcher, such as type AliasAction
| */ | ||
|
|
||
| class EditableAltAliasList extends EditableItemList { | ||
| private _publishedAliasField = React.createRef<Field>(); |
| * TODO: Fixing up this, and AliasSettings, to not need a subclass is a very | ||
| * likely candidate for further cleanup |
There was a problem hiding this comment.
open TODOs are a bit questionable
| /* Don't throw up a dialog if the user closed the parent */ | ||
| if (!isMounted.current) return; | ||
|
|
||
| dispatch({type: ABORT_COMMIT, error: err}); |
There was a problem hiding this comment.
where is this dispatch function coming from?
| class EditableAltAliasList extends EditableItemList { | ||
| private _publishedAliasField = React.createRef<Field>(); | ||
|
|
||
| _onAliasAdded = () => { |
| this._publishedAliasField.current.focus(); | ||
| }; | ||
|
|
||
| _renderNewItemField() { |
I am not super interested in rewriting it. I was mostly reintroducing this PR because it already passed a code review and was just missing a design review. If it needs to be rewritten then it'll probably have to wait for someone else to pick it up. |
|
@aaronraimist I'm taking this one over, hopefully you don't mind. 😄 |
|
@aaronraimist I think this got replaced with #7107 ? Are there remaining bits of this or the linked issue which might still need doing? |
|
@turt2live #7107 is an improvement but in my opinion it doesn't replace this PR. #7107 checks that the address about to be published looked like an alias but this PR actually checks if the alias points at the room and provides a helpful error if not. Currently if you enter a valid looking alias but it doesn't point at the room, you get this error which doesn't explain much. I'm not sure if @Palid wants to leave this open and build on top of it or close it and rewrite it in a different way. |
|
I think if you're able to pick it up instead that would be ideal. |
|
I'll see what I can do but can't promise anything in the near future |
MadLittleMods
added
Z-Community-PR
Attempt 2 at #4574
Step towards fixing element-hq/element-web#13077
TR: Video
Screen.Recording.2021-06-15.at.17.19.05.mov
Here's what your changelog entry will look like:
🐛 Bug Fixes