Page MenuHomePhabricator
Differential D6962

[lib][keyserver] Add editing message type to messageTypes
ClosedPublic
Actions

Authored by • kuba on Mar 7 2023, 1:34 AM.
Tags
None
Referenced Files
Unknown Object (File)
Thu, Apr 3, 1:33 PM
Unknown Object (File)
Thu, Apr 3, 8:34 AM
Unknown Object (File)
Wed, Apr 2, 10:13 PM
Unknown Object (File)
Wed, Apr 2, 4:05 PM
Unknown Object (File)
Wed, Apr 2, 3:12 PM
Unknown Object (File)
Sun, Mar 30, 9:04 PM
Unknown Object (File)
Sun, Mar 23, 4:55 PM
Unknown Object (File)
Mon, Mar 17, 9:21 PM
View All Files
Subscribers
ashoat
atul
tomek

Details

Reviewers
michal
inka
ginsu
Commits
rCOMMa769bb18d4d0: [lib][keyserver] Add editing message type to messageTypes
Summary

Registered new message type EDIT_MESSAGE to messageTypes. Added some lines to prevent flow-typing errors in different places of the app. For now, when we process the added
message type, we ignore it.

Test Plan

Tested if app compiles and checked if everything works correctly.

Diff Detail

Repository
rCOMM Comm
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

• kuba created this revision.Mar 7 2023, 1:34 AM
Herald added subscribers: atul, tomek, ashoat. · View Herald TranscriptMar 7 2023, 1:34 AM
• kuba added a parent revision: D6961: [lib] Create types for message editing.Mar 7 2023, 1:34 AM
• kuba added a child revision: D6963: [lib] Added new MessageSpec for editing messages.Mar 7 2023, 1:40 AM
Harbormaster completed remote builds in B17135: Diff 23475.Mar 7 2023, 1:49 AM
• kuba requested review of this revision.Mar 7 2023, 1:49 AM
• kuba added a reviewer: ginsu.Mar 7 2023, 8:11 AM
ginsu accepted this revision.Mar 9 2023, 10:04 AM

Accepting, but if what I suggested doesn't take too long might be worthwhile to update

lib/shared/messages/sidebar-source-message-spec.js
108–114

When I was working on reactions, @tomek recommended defining a set of incompatible messages with the sidebar source. Now that we have more than two incompatible messages, now might be a good time to address this

https://linear.app/comm/issue/ENG-2422/define-set-of-incompatible-message-types-for-sidebar-source

This revision is now accepted and ready to land.Mar 9 2023, 10:04 AM
inka added a child revision: D7077: [keyserver] Process new messages for search.Mar 15 2023, 4:32 AM
• kuba updated this revision to Diff 24216.Mar 27 2023, 8:32 AM

Rebase to master

lib/shared/messages/sidebar-source-message-spec.js
108–114

Added as follow-up task to message editing

Harbormaster completed remote builds in B17693: Diff 24216.Mar 27 2023, 8:48 AM
Closed by commit rCOMMa769bb18d4d0: [lib][keyserver] Add editing message type to messageTypes (authored by kosmydel <kukubaczek@gmail.com>). · Explain WhyMar 30 2023, 7:23 AM
This revision was automatically updated to reflect the committed changes.
kosmydel <kukubaczek@gmail.com> added a commit: rCOMMa769bb18d4d0: [lib][keyserver] Add editing message type to messageTypes.

Revision Contents
Changeset List

PathSize
keyserver/
src/
creators/
6 lines
fetchers/
5 lines
lib/
selectors/
5 lines
shared/
15 lines
messages/
5 lines
types/
18 lines

Diff 23475

View Options

keyserver/src/creators/thread-creator.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

keyserver/src/fetchers/message-fetchers.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

lib/selectors/chat-selectors.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

lib/shared/message-utils.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

lib/shared/messages/sidebar-source-message-spec.js

Loading...
View Options

lib/types/message-types.js

Loading...