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

Starred Messages View Screen #9182

Closed
wants to merge 18 commits into from

Conversation

yaya-usman
Copy link
Contributor

@yaya-usman yaya-usman commented Aug 12, 2022

Task: vector-im/element-web#22453

Signed-off-by: Yaya Usman [email protected]


Here's what your changelog entry will look like:

✨ Features

@yaya-usman yaya-usman added the T-Enhancement New features, changes in functionality, performance boosts, user-facing improvements label Aug 12, 2022
@yaya-usman yaya-usman requested a review from a team as a code owner August 12, 2022 18:57
@yaya-usman yaya-usman requested a review from germain-gg August 12, 2022 18:57
@yaya-usman yaya-usman removed the request for review from germain-gg August 12, 2022 19:01
@t3chguy
Copy link
Member

t3chguy commented Aug 15, 2022

This needs a user-friendly title otherwise it won't make sense in the changelog

image

src/components/structures/RoomView.tsx Outdated Show resolved Hide resolved
src/stores/RoomViewStore.tsx Outdated Show resolved Hide resolved
src/components/views/rooms/RoomList.tsx Outdated Show resolved Hide resolved
@yaya-usman yaya-usman changed the title Starred_Messages_Feature_Contd_III/Outreachy Starred Message View Page Aug 21, 2022
@yaya-usman yaya-usman changed the title Starred Message View Page Starred Messages View Page Aug 21, 2022
@yaya-usman yaya-usman changed the title Starred Messages View Page Starred Messages View Screen Aug 21, 2022
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.

I haven't had the time to look at most of the components yet but this is mostly looking good, just some smaller concerns

src/PosthogTrackers.ts Outdated Show resolved Hide resolved
res/css/structures/_FavouriteMessagesView.pcss Outdated Show resolved Hide resolved
src/hooks/useFavouriteMessages.ts Outdated Show resolved Hide resolved
Comment on lines +68 to +77
return {
isFavourite,
toggleFavourite,
getFavouriteMessagesIds,
reorderFavouriteMessages,
clearFavouriteMessages,
setSearchState,
isSearchClicked,

};
Copy link
Contributor

Choose a reason for hiding this comment

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

With all these things, it feels like these should be a store rather than a hook perhaps 🤔 When is the switch to account data going to happen?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, i figured out i needed a store instead of this, it's really a pain doing this with hooks only.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Regarding when is the switch to AccountData, i'll start working on it starting next month. For now i think we should stick to this.

res/css/structures/_FavouriteMessagesView.pcss Outdated Show resolved Hide resolved
@yaya-usman yaya-usman marked this pull request as ready for review August 24, 2022 18:44
@yaya-usman yaya-usman marked this pull request as draft August 24, 2022 18:49
@andybalaam
Copy link
Member

I've done a rebase and addressed most of the comments, and created a new PR here: #9719

@andybalaam andybalaam closed this Dec 7, 2022
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