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

Conversation

t3chguy
Copy link
Member

@t3chguy t3chguy commented Dec 11, 2019

Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>
… t3chguy/remove_avatar � Conflicts: �	src/components/views/settings/ProfileSettings.js
Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>
Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>
Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>
Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>
Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>
@t3chguy t3chguy requested a review from a team December 11, 2019 22:49
Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>
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.

it also looks like the design implies that the context menu goes overtop of the box rather than to the right of it

if (e.keyCode === KeyCode.ENTER || e.keyCode === KeyCode.SPACE) {
onChange(!checked);
e.stopPropagation();
e.preventDefault();
Copy link
Member

Choose a reason for hiding this comment

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

unrelated changes should probably be in their own PR for next time

Copy link
Member Author

Choose a reason for hiding this comment

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

Yup

import sdk from "../../../index";

// TODO: Why is rendering a box with an overlay so complicated? Can the DOM be reduced?
const AvatarSetting = (props) => {
Copy link
Member

Choose a reason for hiding this comment

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

ooi why a function with a bunch of bootstrap instead of a tiny PureComponent class?

Copy link
Member Author

Choose a reason for hiding this comment

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

Ability to use useContextMenu hook simplifies things

@t3chguy
Copy link
Member Author

t3chguy commented Dec 16, 2019

Superseded by #3733

@t3chguy t3chguy closed this Dec 16, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

2 participants