Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: add a way to quickly subscribe from search results #713

Merged
merged 50 commits into from
Oct 15, 2023

Conversation

sharunkumar
Copy link
Contributor

@sharunkumar sharunkumar commented Aug 27, 2023

This is especially helpful when using the "Migrate Subreddits" feature

Other features in this PR:

  • long pressing community link has an option to open the sidebar
  • refactor code to have a common hook for community actions useCommunityActions

(Color scheme in the screenshot is from #711)

Screenshot_20231011_180254_Chrome

Screenshot_20231011_180312_Chrome

@SrivatsanSenthilkumar
Copy link
Contributor

SrivatsanSenthilkumar commented Oct 7, 2023

This may be just my subjective opinion but the design does not match with the rest of the UI. Something with a similar design to the star shaped indicator/button used for marking a community as favourite will work better in opinion. A heart shaped indicator (maybe a hollow design with borders to improve indicator visibility) when the community is not subscribed to and solid when you are subscribed to the community.
Screenshot_20231007-130702~2.png

Screenshot_20231007-131837~2.png

@sharunkumar
Copy link
Contributor Author

I just went with the heart because that was the icon used for subscribe when log pressing a community. Open to suggestions from @aeharding

@SrivatsanSenthilkumar
Copy link
Contributor

I want it to be a heart as well. Just that it follows the same design language as the star(that is a empty translucent heart to denote not subscribed and a filled heart to indicate subscribed) for UI design consistency across the app

@sharunkumar
Copy link
Contributor Author

sharunkumar commented Oct 7, 2023

@SrivatsanSenthilkumar I tried it with the same style as the star but it seems like its barely visible in dark mode:

I've committed the changes to another branch for now: sharunkumar@ff1a9d6

if this is fine, ill merge it into the PR

Screenshot_20231007_194722_Chrome

sharunkumar added a commit to sharunkumar/voyager that referenced this pull request Oct 7, 2023
@SrivatsanSenthilkumar
Copy link
Contributor

SrivatsanSenthilkumar commented Oct 8, 2023

Hmmm true. It is barely visible in the dark mode. The star for favouriting has the same issue. Maybe there can be border (if possible the same colour as the accent colour) to improve legibility.

Untitled Note - Oct 8, 2023 11.21.jpg

Please ignore my horrendous drawing skill

Edit : If this looks good maybe I will open an issue related to adding a border on the star as well.

@sharunkumar
Copy link
Contributor Author

sharunkumar commented Oct 9, 2023

Got it, this is what it would look like:

With Outline

Alternatively:

Crossed Heart

not sure which version to add to this PR though 🤔

@SrivatsanSenthilkumar
Copy link
Contributor

Both of them look great. I will open a poll on the voyager community to ascertain which design is preferred.

Also you have my thanks man. You are doing great work here and it's people like you who drive the open source community forward

@SrivatsanSenthilkumar
Copy link
Contributor

Got it, this is what it would look like:

With Outline

Alternatively:

Screenshot_20231008_234638_Chrome

not sure which version to add to this PR though 🤔

The first one is the unanimous winner. Nobody voted for the second design.

@sharunkumar
Copy link
Contributor Author

sharunkumar commented Oct 9, 2023

Nobody voted for the second design.

yeah I had noticed 😅 I'll push the relevant changes to this PR

@sharunkumar
Copy link
Contributor Author

@SrivatsanSenthilkumar @aeharding I could make a similar change to the star icon too:

image

sharunkumar added a commit to sharunkumar/voyager that referenced this pull request Oct 10, 2023
@aeharding
Copy link
Owner

Hey, thanks for the PR! So I'm just taking my initial review of this. Regarding the UI, I'd like to match what we do for coloring/filled state for the star UI on the existing communities list page. I think it would be good to keep this consistent - and this is the UI that Apollo has. (The greyed out stars also reduce noise in the UI)

@sharunkumar
Copy link
Contributor Author

I haven't updated the stars in the UI, it was just a mock up using local dev changes. Should I change the hearts to be faded too?

@aeharding
Copy link
Owner

I can take a look shortly, but yeah! :)


# fly.io
fly.toml

Choose a reason for hiding this comment

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

Did you mean to include this fly.toml exclusion?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

to prevent myself from accidentally committing mine to this PR ;)

Copy link
Contributor Author

@sharunkumar sharunkumar left a comment

Choose a reason for hiding this comment

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

image

Not sure what's causing this, but when I navigate to a community page, the console shows this error

@aeharding
Copy link
Owner

@sharunkumar I removed the local state from the useCommunityActions file. Instead I use the redux source of truth. The reason why it wasn't working is the data received from the search API for the community list wasn't populating the redux store. Check it out and let me know what you think

@sharunkumar
Copy link
Contributor Author

@sharunkumar I removed the local state from the useCommunityActions file. Instead I use the redux source of truth. The reason why it wasn't working is the data received from the search API for the community list wasn't populating the redux store. Check it out and let me know what you think

got it, I checked it in my local and it seems to be working as expected 👍

@aeharding aeharding merged commit 57f4854 into aeharding:main Oct 15, 2023
2 checks passed
@sharunkumar
Copy link
Contributor Author

🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants