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

Starred_Messages_Feature/Outreachy #8842

Merged
merged 1 commit into from
Jul 15, 2022

Conversation

yaya-usman
Copy link
Contributor

@yaya-usman yaya-usman commented Jun 14, 2022

Task: vector-im/element-web#22453

Signed-off-by: Yaya Usman yaya-usman@users.noreply.github.com

Type: enhancement


Here's what your changelog entry will look like:

✨ Features

  • Starred_Messages_Feature/Outreachy (#8842).

@yaya-usman yaya-usman requested a review from a team as a code owner June 14, 2022 14:21
@github-actions github-actions bot added the T-Enhancement New features, changes in functionality, performance boosts, user-facing improvements label Jun 14, 2022
@yaya-usman yaya-usman requested a review from andybalaam June 14, 2022 14:24
@yaya-usman yaya-usman self-assigned this Jun 14, 2022
@t3chguy t3chguy marked this pull request as draft June 14, 2022 14:33
Copy link
Member

@t3chguy t3chguy left a comment

Choose a reason for hiding this comment

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

Clearing review while WIP, you can iterate on it with @andybalaam

Copy link
Member

@andybalaam andybalaam left a comment

Choose a reason for hiding this comment

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

This looks great. Setting my review to "Request changes" because we agreed that we will wait until the feature actually does something before merging.

Copy link
Member

@andybalaam andybalaam left a comment

Choose a reason for hiding this comment

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

This looks really good. I don't understand all that useEffect stuff so we'll need someone else to review this, but when you have done all the things I commented on, we should be ready for someone else to review.

The hardest thing I asked for is more tests of the saving code. Feel free to ask me for help on that.

src/components/views/messages/MessageActionBar.tsx Outdated Show resolved Hide resolved
src/contexts/FavMessageContext.tsx Outdated Show resolved Hide resolved
src/contexts/FavMessageContext.tsx Outdated Show resolved Hide resolved
src/contexts/FavMessageContext.tsx Outdated Show resolved Hide resolved
src/dispatcher/actions.ts Outdated Show resolved Hide resolved
src/contexts/FavMessageContext.tsx Outdated Show resolved Hide resolved
src/contexts/FavMessageContext.tsx Outdated Show resolved Hide resolved
src/contexts/FavMessageContext.tsx Outdated Show resolved Hide resolved
src/components/views/messages/MessageActionBar.tsx Outdated Show resolved Hide resolved
res/css/views/messages/_MessageActionBar.scss Outdated Show resolved Hide resolved
Copy link
Contributor

@SimonBrandner SimonBrandner left a comment

Choose a reason for hiding this comment

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

@yaya-usman
Copy link
Contributor Author

Needs to be added to https://github.com/vector-im/element-web/blob/develop/docs/labs.md

Got it

@yaya-usman yaya-usman requested a review from t3chguy July 6, 2022 13:42
@t3chguy
Copy link
Member

t3chguy commented Jul 6, 2022

This has conflicts and changes requested, please request a review once it is ready for review

Copy link
Member

@andybalaam andybalaam left a comment

Choose a reason for hiding this comment

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

Things left to resolve:

  • labs.md
  • unremove that blank line
  • Change the default export of FavouriteMessageContext.tsx to FavouriteMessageContext

src/dispatcher/actions.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@SimonBrandner SimonBrandner left a comment

Choose a reason for hiding this comment

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

Looking much better - thank you for the updates!

src/components/views/messages/MessageActionBar.tsx Outdated Show resolved Hide resolved
src/components/views/messages/MessageActionBar.tsx Outdated Show resolved Hide resolved
src/hooks/useFavouriteMessages.ts Show resolved Hide resolved
Copy link
Member

@andybalaam andybalaam left a comment

Choose a reason for hiding this comment

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

Looks good!

Copy link
Contributor

@SimonBrandner SimonBrandner left a comment

Choose a reason for hiding this comment

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

After further thought even though the way localStorage is used here isn't ideal, we can ignore it as it will soon be replaced

LGTM - thank you!

localStorage.setItem('io_element_favouriteMessages', JSON.stringify(favouriteMessageIds));

//force update
setX([]);
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you please add a comment about why is this necessary

@t3chguy t3chguy dismissed their stale review July 15, 2022 13:55

stale

@t3chguy t3chguy removed their request for review July 15, 2022 13:55
@yaya-usman yaya-usman enabled auto-merge July 15, 2022 19:39
Copy link
Contributor

@SimonBrandner SimonBrandner left a comment

Choose a reason for hiding this comment

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

Thank you!

(will let Andy press the button; also, please fix the merge conflicts)

@yaya-usman
Copy link
Contributor Author

Thank you!

(will let Andy press the button; also, please fix the merge conflicts)

Thanks but i don't know why i couldn't, it's weird

@yaya-usman
Copy link
Contributor Author

conflict

@SimonBrandner
Copy link
Contributor

conflict

You need to manually resolve them in your IDE/code editor

@yaya-usman
Copy link
Contributor Author

conflict

This is what i meant, i don't know why i couldn't click the button, also i don't have any conflicts in my local version. So it's weird

@yaya-usman
Copy link
Contributor Author

Thank you!

(will let Andy press the button; also, please fix the merge conflicts)

@SimonBrandner Andy asked me to click merge if i made the changes you requested and the checks passes. I hope it's ok by you. Also Thanks for the review!

@SimonBrandner
Copy link
Contributor

Thank you!
(will let Andy press the button; also, please fix the merge conflicts)

@SimonBrandner Andy asked me to click merge if i made the changes you requested and the checks passes. I hope it's ok by you. Also Thanks for the review!

Sounds good 👍

@yaya-usman yaya-usman merged commit 48552c2 into matrix-org:develop Jul 15, 2022
@yaya-usman yaya-usman deleted the star_feature branch July 15, 2022 20:30
su-ex added a commit to SchildiChat/element-desktop that referenced this pull request Aug 22, 2022
* Live location share -  focus on user location on list item click ([\#9051](matrix-org/matrix-react-sdk#9051)). Contributed by @kerryarchibald.
* Live location sharing - don't trigger unread counts for beacon location events ([\#9071](matrix-org/matrix-react-sdk#9071)). Contributed by @kerryarchibald.
* Support for sending voice messages as replies and in threads ([\#9097](matrix-org/matrix-react-sdk#9097)). Fixes element-hq/element-web#22031.
* Add `Reply in thread` button to the right-click message context-menu ([\#9004](matrix-org/matrix-react-sdk#9004)). Fixes element-hq/element-web#22745.
* Starred_Messages_Feature_Contd_II/Outreachy ([\#9086](matrix-org/matrix-react-sdk#9086)).
* Use "frequently used emojis" for autocompletion in composer ([\#8998](matrix-org/matrix-react-sdk#8998)). Fixes element-hq/element-web#18978. Contributed by @grimhilt.
* Improve clickability of view source event toggle button  ([\#9068](matrix-org/matrix-react-sdk#9068)). Fixes element-hq/element-web#21856. Contributed by @luixxiul.
* Improve clickability of "collapse" link button on bubble layout ([\#9037](matrix-org/matrix-react-sdk#9037)). Fixes element-hq/element-web#22864. Contributed by @luixxiul.
* Starred_Messages_Feature/Outreachy ([\#8842](matrix-org/matrix-react-sdk#8842)).
* Implement Use Case Selection screen ([\#8984](matrix-org/matrix-react-sdk#8984)). Contributed by @justjanne.
* Live location share - handle insufficient permissions in location sharing ([\#9047](matrix-org/matrix-react-sdk#9047)). Contributed by @kerryarchibald.
* Improve _FilePanel.scss ([\#9031](matrix-org/matrix-react-sdk#9031)). Contributed by @luixxiul.
* Improve spotlight accessibility by adding context menus ([\#8907](matrix-org/matrix-react-sdk#8907)). Fixes element-hq/element-web#20875 and element-hq/element-web#22675. Contributed by @justjanne.
* Replace mask-images with svg components in MessageActionBar ([\#9088](matrix-org/matrix-react-sdk#9088)). Fixes element-hq/element-web#22912. Contributed by @kerryarchibald.
* Unbreak in-app permalink tooltips ([\#9087](matrix-org/matrix-react-sdk#9087)). Fixes element-hq/element-web#22874.
* Show a back button when viewing a space member ([\#9095](matrix-org/matrix-react-sdk#9095)). Fixes element-hq/element-web#22898.
* Align the right edge of info tile lines with normal ones on IRC layout ([\#9058](matrix-org/matrix-react-sdk#9058)). Fixes element-hq/element-web#22871. Contributed by @luixxiul.
* Prevent email verification from overriding existing sessions ([\#9075](matrix-org/matrix-react-sdk#9075)). Fixes element-hq/element-web#22881. Contributed by @justjanne.
* Fix wrong buttons being used when exploring public rooms ([\#9062](matrix-org/matrix-react-sdk#9062)). Fixes element-hq/element-web#22862.
* Re-add padding to generic event list summary on IRC layout ([\#9063](matrix-org/matrix-react-sdk#9063)). Fixes element-hq/element-web#22869. Contributed by @luixxiul.
* Joining federated rooms via the spotlight search should no longer cause a "No known servers" error. ([\#9055](matrix-org/matrix-react-sdk#9055)). Fixes element-hq/element-web#22845. Contributed by @Half-Shot.
su-ex added a commit to SchildiChat/matrix-react-sdk that referenced this pull request Aug 22, 2022
* Live location share -  focus on user location on list item click ([\matrix-org#9051](matrix-org#9051)). Contributed by @kerryarchibald.
* Live location sharing - don't trigger unread counts for beacon location events ([\matrix-org#9071](matrix-org#9071)). Contributed by @kerryarchibald.
* Support for sending voice messages as replies and in threads ([\matrix-org#9097](matrix-org#9097)). Fixes element-hq/element-web#22031.
* Add `Reply in thread` button to the right-click message context-menu ([\matrix-org#9004](matrix-org#9004)). Fixes element-hq/element-web#22745.
* Starred_Messages_Feature_Contd_II/Outreachy ([\matrix-org#9086](matrix-org#9086)).
*  Use "frequently used emojis" for autocompletion in composer ([\matrix-org#8998](matrix-org#8998)). Fixes element-hq/element-web#18978. Contributed by @grimhilt.
* Improve clickability of view source event toggle button  ([\matrix-org#9068](matrix-org#9068)). Fixes element-hq/element-web#21856. Contributed by @luixxiul.
* Improve clickability of "collapse" link button on bubble layout ([\matrix-org#9037](matrix-org#9037)). Fixes element-hq/element-web#22864. Contributed by @luixxiul.
* Starred_Messages_Feature/Outreachy ([\matrix-org#8842](matrix-org#8842)).
* Implement Use Case Selection screen ([\matrix-org#8984](matrix-org#8984)). Contributed by @justjanne.
* Live location share - handle insufficient permissions in location sharing ([\matrix-org#9047](matrix-org#9047)). Contributed by @kerryarchibald.
* Improve _FilePanel.scss ([\matrix-org#9031](matrix-org#9031)). Contributed by @luixxiul.
* Improve spotlight accessibility by adding context menus ([\matrix-org#8907](matrix-org#8907)). Fixes element-hq/element-web#20875 and element-hq/element-web#22675. Contributed by @justjanne.
* Replace mask-images with svg components in MessageActionBar ([\matrix-org#9088](matrix-org#9088)). Fixes element-hq/element-web#22912. Contributed by @kerryarchibald.
* Unbreak in-app permalink tooltips ([\matrix-org#9087](matrix-org#9087)). Fixes element-hq/element-web#22874.
* Show a back button when viewing a space member ([\matrix-org#9095](matrix-org#9095)). Fixes element-hq/element-web#22898.
* Align the right edge of info tile lines with normal ones on IRC layout ([\matrix-org#9058](matrix-org#9058)). Fixes element-hq/element-web#22871. Contributed by @luixxiul.
* Prevent email verification from overriding existing sessions ([\matrix-org#9075](matrix-org#9075)). Fixes element-hq/element-web#22881. Contributed by @justjanne.
* Fix wrong buttons being used when exploring public rooms ([\matrix-org#9062](matrix-org#9062)). Fixes element-hq/element-web#22862.
* Re-add padding to generic event list summary on IRC layout ([\matrix-org#9063](matrix-org#9063)). Fixes element-hq/element-web#22869. Contributed by @luixxiul.
* Joining federated rooms via the spotlight search should no longer cause a "No known servers" error. ([\matrix-org#9055](matrix-org#9055)). Fixes element-hq/element-web#22845. Contributed by @Half-Shot.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
T-Enhancement New features, changes in functionality, performance boosts, user-facing improvements
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants