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: allow user to view follow list in "Hide following/follower count" mode #1901

Merged
merged 5 commits into from
Apr 16, 2023

Conversation

kkoyung
Copy link
Contributor

@kkoyung kkoyung commented Mar 17, 2023

From @patak-dev 's idea (see here), I adjusted the wellbeing "Hide following/follower count" feature.

Before changes, when "Hide following/follower count" is enabled, the following count and the follower count are hidden as shown below.
screenshot_2023-03-17_23:13:49

After changes, when "Hide following/follower count" is enabled, only the numbers are hidden. The text "Following" and "Followers" are kept so that users are still able to click them to view the following list and the follower list.
screenshot_2023-03-17_23:15:03

@stackblitz
Copy link

stackblitz bot commented Mar 17, 2023

Review PR in StackBlitz Codeflow Run & review this pull request in StackBlitz Codeflow.

@netlify
Copy link

netlify bot commented Mar 17, 2023

Deploy Preview for elk-zone ready!

Name Link
🔨 Latest commit e94d541
🔍 Latest deploy log https://app.netlify.com/sites/elk-zone/deploys/64171300e7b4000008f80f2e
😎 Deploy Preview https://deploy-preview-1901--elk-zone.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@netlify
Copy link

netlify bot commented Mar 17, 2023

Deploy Preview for elk-docs canceled.

Name Link
🔨 Latest commit e94d541
🔍 Latest deploy log https://app.netlify.com/sites/elk-docs/deploys/64171300351b3a0008fbaafc

@patak-dev
Copy link
Member

Nice! Thanks for the PR 🙌🏼

Two things I see:

  • We could avoid showing Following/Followers when you are out of the profile (so it doesn't show when hovering on an avatar for example)
  • I think it is better to have new keys for Following/Followers without a number or force it to use the plural form to avoid seeing Follower if there is only 1.

@kkoyung
Copy link
Contributor Author

kkoyung commented Mar 18, 2023

I made changes on them.

  • In the AccountPostFollowers component, I added a props isHoverCard which is set to be true by AccountHoverCard so that "Following" and "Followers" will be further hidden in hover card.

  • In the LocalizedNumber component, force the plural form when the count is hidden, by :plural="hideNumber ? 2 : count".

@antfu
Copy link
Member

antfu commented Mar 19, 2023

I would also suggest we have separate keys for non-numbered names. The plural workaround works for English but is not necessary for other languages.

@kkoyung
Copy link
Contributor Author

kkoyung commented Mar 19, 2023

You are right. Simply taking away the number may not work in some languages. I changed to re-use account.following and account.followers when counts are hidden. These two keys are used as html title after clicking "Following"/"Followers". I think it makes sense to share same text between the button and the page title after clicking the button.

Besides that, I found that when I click "XX Posts", "XX Following" and "XX Followers" back and forth, the page title only changes once for each one. I will create another issue about that later.

Copy link
Member

@patak-dev patak-dev left a comment

Choose a reason for hiding this comment

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

Sorry, it took us some time to merge this one. Looks great to me now! Thanks for implementing the feature.

@patak-dev patak-dev changed the title chore: Allow user to view follow list in "Hide following/follower count" mode feat: allow user to view follow list in "Hide following/follower count" mode Apr 16, 2023
@patak-dev patak-dev merged commit ca8d785 into elk-zone:main Apr 16, 2023
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.

3 participants