Page MenuHomePhabricator
Differential D8602

Remove aes-crypto-utils.js from media and use common API to avoid redundent layer of abstraction
ClosedPublic
Actions

Authored by marcin on Jul 24 2023, 4:24 AM.
Tags
None
Referenced Files
Unknown Object (File)
Tue, Apr 1, 10:28 PM
Unknown Object (File)
Thu, Mar 27, 10:08 PM
Unknown Object (File)
Thu, Mar 27, 6:10 AM
Unknown Object (File)
Wed, Mar 19, 4:14 PM
Unknown Object (File)
Tue, Mar 18, 12:41 PM
Unknown Object (File)
Mar 6 2025, 5:44 AM
Unknown Object (File)
Mar 5 2025, 7:57 AM
Unknown Object (File)
Mar 4 2025, 8:46 PM
View All Files
Subscribers
ashoat
tomek

Details

Reviewers
bartek
Commits
rCOMMe6ba46a93134: Remove aes-crypto-utils.js from media and use common API to avoid redundent…
Summary

This differential removes web/media/aes-crypto-utils.js file and changes relevant files to make direct calls to lib/media/aes-crypto-utils.js API. This change was done in response to one of the comments to parent differential

Test Plan
  1. Run flow. 2. Ensure that CI passes (it runs jest)

Diff Detail

Repository
rCOMM Comm
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

marcin created this revision.Jul 24 2023, 4:24 AM
Herald added subscribers: tomek, ashoat. · View Herald TranscriptJul 24 2023, 4:24 AM
marcin updated this revision to Diff 28958.Jul 24 2023, 4:26 AM

Rebase to update commit message

Harbormaster returned this revision to the author for changes because remote builds failed.Jul 24 2023, 4:39 AM
Harbormaster failed remote builds in B21136: Diff 28958!
Harbormaster completed remote builds in B21135: Diff 28957.Jul 24 2023, 4:41 AM
marcin added a parent revision: D8521: Enable webcrypto usage in keyserver codebase and unify aes crypto stack across web and the keyserver.Jul 24 2023, 4:45 AM
marcin added a child revision: D8529: Implement utility function on the keyserver to upload notification payload to blob service.
Harbormaster completed remote builds in B21136: Diff 28958.Jul 24 2023, 5:04 AM
marcin requested review of this revision.Jul 24 2023, 5:04 AM
bartek accepted this revision.Jul 25 2023, 3:05 AM
This revision is now accepted and ready to land.Jul 25 2023, 3:05 AM
marcin updated this revision to Diff 33416.Nov 20 2023, 5:42 AM

Rebase before landing

Harbormaster completed remote builds in B24278: Diff 33416.Nov 20 2023, 6:37 AM
Closed by commit rCOMMe6ba46a93134: Remove aes-crypto-utils.js from media and use common API to avoid redundent… (authored by marcin). · Explain WhyNov 20 2023, 7:09 AM
This revision was automatically updated to reflect the committed changes.
marcin added a commit: rCOMMe6ba46a93134: Remove aes-crypto-utils.js from media and use common API to avoid redundent….

Revision Contents
Changeset List

Diff 28958

View Options

web/media/aes-crypto-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

web/media/aes-crypto-utils.test.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/media/encryption-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

web/media/image-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