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 'use star favorite icon' preference #2067

Merged
merged 5 commits into from
May 6, 2023
Merged

feat: add 'use star favorite icon' preference #2067

merged 5 commits into from
May 6, 2023

Conversation

shuuji3
Copy link
Member

@shuuji3 shuuji3 commented May 5, 2023

resolves #2058

This will add an additional option to change the favorite icon.

This will change the color of the icon from rose to yellow but this causes the same color usage as the bookmark icon.

Screenshot 2023-05-05 at 20 06 20

@stackblitz
Copy link

stackblitz bot commented May 5, 2023

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

@netlify
Copy link

netlify bot commented May 5, 2023

Deploy Preview for elk-docs canceled.

Name Link
🔨 Latest commit f4d030d
🔍 Latest deploy log https://app.netlify.com/sites/elk-docs/deploys/6455b6823e32be0007d8bc66

@shuuji3 shuuji3 changed the title feat: add 'use start favorite icon' preference feat: add 'use star favorite icon' preference May 5, 2023
@netlify
Copy link

netlify bot commented May 5, 2023

Deploy Preview for elk-zone ready!

Name Link
🔨 Latest commit f4d030d
🔍 Latest deploy log https://app.netlify.com/sites/elk-zone/deploys/6455b6823130110008491311
😎 Deploy Preview https://deploy-preview-2067--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.

@Shinigami92
Copy link
Member

This might bite with favorising tags:

image

@shuuji3
Copy link
Member Author

shuuji3 commented May 5, 2023

That's true. I think we can distinguish the behavior with the help of tooltips and the user will be intentional to enable this feature. Similarly, we already have the same heart icons between the favorite and right-bottom donations but there is no confusion for them.

Screenshot 2023-05-05 at 22 49 06

Screenshot 2023-05-05 at 22 49 27

@patak-dev
Copy link
Member

Nice! Two details, we could use red for bookmark so it isnt yellow like the star. So swap the colors if the new option is enabled.

And we should add a new env variable to let people hosting the app which should be the default. At least there is one server that would like star by default. We could do this in another PR thought

@shuuji3
Copy link
Member Author

shuuji3 commented May 5, 2023

Nice! Two details, we could use red for bookmark so it isnt yellow like the star. So swap the colors if the new option is enabled.

That's a good idea! 🙂 I'll update the PR soon.

As for the environment variable, maybe I can work on that. But I'm not familiar with those settings yet, so I'll leave that to another PR.

@shuuji3
Copy link
Member Author

shuuji3 commented May 5, 2023

Updated the bookmark icon color.

Screenshot 2023-05-06 at 1 30 38

Copy link
Member

@Shinigami92 Shinigami92 left a comment

Choose a reason for hiding this comment

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

If getPreferences(userSettings, 'useStarFavoriteIcon') is used multiple times in a component, should it be extracted into a computed to safe performance?

@patak-dev
Copy link
Member

I don't think it will affect performance, but I agree it is better to extract it 👍🏼

@shuuji3
Copy link
Member Author

shuuji3 commented May 5, 2023

Thanks, I tried the usePreferences() to get computed for useStarFavoriteIcon. This also could simplify some templates.

@Shinigami92 Shinigami92 added the c: feature Request for new feature label May 5, 2023
@@ -149,8 +150,8 @@ function showFavoritedAndBoostedBy() {

<CommonDropdownItem
:text="status.favourited ? $t('action.favourited') : $t('action.favourite')"
:icon="status.favourited ? 'i-ri:heart-3-fill' : 'i-ri:heart-3-line'"
:class="status.favourited ? 'text-rose' : ''"
:icon="`${useStarFavoriteIcon ? 'i-ri:star-' : 'i-ri:heart-3-'}${status.favourited ? 'fill' : 'line'}`"
Copy link
Member

Choose a reason for hiding this comment

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

You should try to always have the classes as fully written out and not dynamically calculated and concatinated because this results in issues

try something like

useStarFavoriteIcon
  ? status.favourited ? 'i-ri:star-fill'    : 'i-ri:star-line'
  : status.favourited ? 'i-ri:heart-3-fill' : 'i-ri:heart-3-line'

this also applies to lines below

Copy link
Member Author

Choose a reason for hiding this comment

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

OK, this was fixed by a new commit. 👍🏻

Copy link
Member

@Shinigami92 Shinigami92 left a comment

Choose a reason for hiding this comment

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

Another one should also approve 🙂

@Shinigami92 Shinigami92 requested a review from patak-dev May 6, 2023 08:53
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.

Awesome 🌟

@patak-dev patak-dev merged commit 582a984 into elk-zone:main May 6, 2023
@shuuji3 shuuji3 deleted the feat/star-fav-icon branch May 6, 2023 15:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c: feature Request for new feature
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Option to use a star icon instead of a heart icon for favourites
4 participants