Apply new design and display logic to logout confirmation dialog#33426
Apply new design and display logic to logout confirmation dialog#33426uhoreg wants to merge 16 commits into
Conversation
uhoreg
force-pushed
the
logout_dialog_redesign
branch
from
a3b6754 to
7631cbf
Compare
|
Spacing between heading and text looks wrong compared to Figma 
|
Added some spacing |
|
Not sure why one of the playwright tests is still failing, but this should otherwise be ready for review. |
It is looking for a button which does not seem to exist 
|
|
Right, because it has another device in that test, so it's just showing a logout confirmation, rather than a nag about setting up recovery. |
|
HI! I'll take a look once the feature is approved by the crypto team |
|
@florianduros this is ready for you to review, thanks! |
| onFinished: function () {}, | ||
| }; | ||
| export default function LogoutDialog(props: IProps): JSX.Element { | ||
| const client = MatrixClientPeg.safeGet(); |
There was a problem hiding this comment.
| const client = MatrixClientPeg.safeGet(); | |
| const client = useMatrixClientContext(); |
There was a problem hiding this comment.
I had tried this, but useMatrixClientContext seems to be returning undefined. So somehow the context isn't passed through properly. But I don't know much about contexts.
There was a problem hiding this comment.
It means that the dialog is not wrapped in a MatrixClient context provider
There was a problem hiding this comment.
Looking at where this dialog gets used, it's created (in 2 places) by a call to Modal.createDialog(), which I think means that if I want to wrap it in a MatrixClient context provider, means that I would need to modify the Modal component to somehow receive a MatrixClient and pass it on to child components. Which ... I'm not sure how comfortable I would be to do that, given that Modal is used in other places, and also seems like beyond the scope of this PR.
I could change it so that LogoutDialog takes the client as a property, though, if that would be better.
| <p>{_t("auth|logout_dialog|setup_secure_backup_description")}</p> | ||
| <div> | ||
| <a target="_blank" href="https://element.io/en/help#encryption16"> | ||
| {_t("action|learn_more")} <PopOutIcon /> | ||
| </a> | ||
| </div> |
There was a problem hiding this comment.
For text prefer typography component of compound cf https://github.com/element-hq/element-web/blob/develop/code_style.md#react
If you can't, be sure to apply the correct letter spacing
There was a problem hiding this comment.
That part of the doc links to https://compound.element.io/?path=%2Fdocs%2Fcompound-web_typography--docs, which only seems to offer the <Heading> component, which probably isn't what we want here. The docs also don't say how to "apply the correct CSS classes for font and letter spacing". Where can I learn more about that.
So, I did originally try to use the <Link> component for this, but <Link> makes the link black, whereas the design calls for it to be blue. What's the preferred way of doing this?
There was a problem hiding this comment.
There is a <Text /> component https://compound.element.io/?path=/story/compound-web_typography--text that you can use both p and a marker.
The docs also don't say how to "apply the correct CSS classes for font and letter spacing".
For a md font: font-letter-spacing: var(--cpd-font-letter-spacing-body-md). However I strongly encourage to use the compound typography component because it both applies the font and the letter spacing
There was a problem hiding this comment.
Thanks. I've changed it to use <Text>. Hopefully I did it right.
| <Text> | ||
| <a target="_blank" href="https://element.io/en/help#encryption16"> | ||
| {_t("action|learn_more")} <PopOutIcon /> | ||
| </a> | ||
| </div> | ||
| </Text> |
There was a problem hiding this comment.
Why do we need a div here (there is no class on it)?
We can use <Text as="a"> to have a tag with the Text properties
There was a problem hiding this comment.
OK, I've changed it to use <Text as="a">
4 participants
Part of element-hq/element-meta#2834
Applies the new design to the dialog that prompts the user to set up recovery when they try to log out.
Also applies new logic for determining when to show the prompt:
I've also taking this opportunity to switch to a functional component, and move the non-display code into hooks.
This PR does not implement allowing the user to get their recovery key in-line (it just brings them to the settings dialog).
Checklist
public/exportedsymbols have accurate TSDoc documentation.