Page MenuHomePhabricator
Differential D8286

[web] Remove `LoadingIndicator` from `EditUserAvatar`
ClosedPublic
Actions

Authored by atul on Jun 21 2023, 3:39 PM.
Tags
None
Referenced Files
Unknown Object (File)
Tue, Apr 1, 7:50 AM
Unknown Object (File)
Mon, Mar 31, 5:22 PM
Unknown Object (File)
Sun, Mar 30, 9:04 PM
Unknown Object (File)
Sat, Mar 15, 4:35 PM
Unknown Object (File)
Mar 5 2025, 4:02 PM
Unknown Object (File)
Mar 5 2025, 4:02 PM
Unknown Object (File)
Mar 5 2025, 4:02 PM
Unknown Object (File)
Mar 5 2025, 4:01 PM
View All Files
Subscribers
tomek

Details

Reviewers
ashoat
ginsu
rohan
Commits
rCOMMce4888d64b5f: [web] Remove `LoadingIndicator` from `EditUserAvatar`
Summary

In D8285 we introduced a showSpinner prop to the Avatar component. In this diff we remove the LoadingIndicator from EditUserAvatar component and instead pass showSpinner down to Avatar to take advantage of that.

This deduplicates redundant spinner code. Now that we have a "toggleable" spinner in Avatar, components derived from it can use that spinner "for free."

Depends on D8285

Test Plan

Things continue to look as expected:

Diff Detail

Repository
rCOMM Comm
Branch
master
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

atul created this revision.Jun 21 2023, 3:39 PM
Herald added a subscriber: tomek. · View Herald TranscriptJun 21 2023, 3:39 PM
atul published this revision for review.Jun 21 2023, 3:40 PM
atul edited the test plan for this revision. (Show Details)
atul edited the summary of this revision. (Show Details)
atul edited the summary of this revision. (Show Details)
Harbormaster completed remote builds in B20413: Diff 27985.Jun 21 2023, 4:03 PM
atul added a child revision: D8287: [web] Split `EmojiAvatarSelectionModal` into "Emoji" and "Color" tab.Jun 21 2023, 4:04 PM
rohan accepted this revision.Jun 22 2023, 6:29 AM
This revision is now accepted and ready to land.Jun 22 2023, 6:29 AM
atul updated this revision to Diff 28082.Jun 23 2023, 1:56 PM

rebase before landing

This revision was landed with ongoing or failed builds.Jun 23 2023, 2:07 PM
Closed by commit rCOMMce4888d64b5f: [web] Remove `LoadingIndicator` from `EditUserAvatar` (authored by atul). · Explain Why
This revision was automatically updated to reflect the committed changes.
atul added a commit: rCOMMce4888d64b5f: [web] Remove `LoadingIndicator` from `EditUserAvatar`.
Harbormaster completed remote builds in B20496: Diff 28082.Jun 23 2023, 2:45 PM

Revision Contents
Changeset List

Diff 27985

View Options

web/avatars/edit-user-avatar.css

  • This file was added.
div.containerViewerLiked {
padding: 4px;
border-radius: 4px;
background-color: var(--shades-black-80);
}
div.container:hover {
cursor: pointer;
}
tomekUnsubmitted
Not Done
Inline Actions

For other tooltip actions we don't need to manually set this style. Can we use the mechanism we already have, or are there any reasons for not doing so?

tomek: For other tooltip actions we don't need to manually set this style. Can we use the mechanism we…
ginsuAuthorUnsubmitted
Done
Inline Actions

I believe the other icons don't need to set this style because it is built into IcomoonReact. When I switched the Like action tooltip button with one of the icons the hover style just works out of the box. Wondering if @atul has more context since he is pretty familiar with the icons stuff

ginsu: I believe the other icons don't need to set this style because it is built into `IcomoonReact`.
tomekUnsubmitted
Not Done
Inline Actions

It wouldn't make too much sense for Icomoon to provide hover effects for icons because they could be used in other places - not only as buttons. It seems like the style on hover is applied in message-tooltip.css

div.messageActionButtons svg:hover {
  cursor: pointer;
  color: var(--fg);
}

so it should be enough to apply this style also to the new button.

tomek: It wouldn't make too much sense for `Icomoon` to provide hover effects for icons because they…
ginsuAuthorUnsubmitted
Done
Inline Actions

Ahhhhh that makes sense, thank you for clarifying, I will make the necessary changes

ginsu: Ahhhhh that makes sense, thank you for clarifying, I will make the necessary changes
View Options

web/avatars/edit-user-avatar.react.js

  • This file was added.
div.containerViewerLiked {
padding: 4px;
border-radius: 4px;
background-color: var(--shades-black-80);
}
div.container:hover {
cursor: pointer;
}
tomekUnsubmitted
Not Done
Inline Actions

For other tooltip actions we don't need to manually set this style. Can we use the mechanism we already have, or are there any reasons for not doing so?

tomek: For other tooltip actions we don't need to manually set this style. Can we use the mechanism we…
ginsuAuthorUnsubmitted
Done
Inline Actions

I believe the other icons don't need to set this style because it is built into IcomoonReact. When I switched the Like action tooltip button with one of the icons the hover style just works out of the box. Wondering if @atul has more context since he is pretty familiar with the icons stuff

ginsu: I believe the other icons don't need to set this style because it is built into `IcomoonReact`.
tomekUnsubmitted
Not Done
Inline Actions

It wouldn't make too much sense for Icomoon to provide hover effects for icons because they could be used in other places - not only as buttons. It seems like the style on hover is applied in message-tooltip.css

div.messageActionButtons svg:hover {
  cursor: pointer;
  color: var(--fg);
}

so it should be enough to apply this style also to the new button.

tomek: It wouldn't make too much sense for `Icomoon` to provide hover effects for icons because they…
ginsuAuthorUnsubmitted
Done
Inline Actions

Ahhhhh that makes sense, thank you for clarifying, I will make the necessary changes

ginsu: Ahhhhh that makes sense, thank you for clarifying, I will make the necessary changes
View Options

web/avatars/user-avatar.react.js

  • This file was added.
div.containerViewerLiked {
padding: 4px;
border-radius: 4px;
background-color: var(--shades-black-80);
}
div.container:hover {
cursor: pointer;
}
tomekUnsubmitted
Not Done
Inline Actions

For other tooltip actions we don't need to manually set this style. Can we use the mechanism we already have, or are there any reasons for not doing so?

tomek: For other tooltip actions we don't need to manually set this style. Can we use the mechanism we…
ginsuAuthorUnsubmitted
Done
Inline Actions

I believe the other icons don't need to set this style because it is built into IcomoonReact. When I switched the Like action tooltip button with one of the icons the hover style just works out of the box. Wondering if @atul has more context since he is pretty familiar with the icons stuff

ginsu: I believe the other icons don't need to set this style because it is built into `IcomoonReact`.
tomekUnsubmitted
Not Done
Inline Actions

It wouldn't make too much sense for Icomoon to provide hover effects for icons because they could be used in other places - not only as buttons. It seems like the style on hover is applied in message-tooltip.css

div.messageActionButtons svg:hover {
  cursor: pointer;
  color: var(--fg);
}

so it should be enough to apply this style also to the new button.

tomek: It wouldn't make too much sense for `Icomoon` to provide hover effects for icons because they…
ginsuAuthorUnsubmitted
Done
Inline Actions

Ahhhhh that makes sense, thank you for clarifying, I will make the necessary changes

ginsu: Ahhhhh that makes sense, thank you for clarifying, I will make the necessary changes