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

Allow Hiding Skill's Percentage #87

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

LaQuay
Copy link

@LaQuay LaQuay commented Oct 10, 2021

This gives the possibility to the developer of deciding if the % is shown or not.

Copy link
Owner

@yousinix yousinix left a comment

Choose a reason for hiding this comment

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

Instead of adding another showPercentage key, I would recommend using the existing percentage key by hiding the percentage number when this key is not defined or empty. This approach is not just cleaner, but it prevents breaking existing sites.

Example:

- name: Skill X
  percentage: 90
  color: danger
  
# Percentage won't show
- name: Skill Y
  color: danger
 
# Percentage won't show
- name: Skill Z
  percentage: 
  color: danger

@yousinix yousinix changed the title add possibility to hide the percentage number in skills Allow Hiding Skill's Percentage Oct 10, 2021
@LaQuay
Copy link
Author

LaQuay commented Oct 11, 2021

Hi, thanks for your fast response.
Maybe I did not explain it very well. The idea of this PR is to let the user have the possibility of having the bar with a percentage but hiding the percentage label.

If your concern is more about preventing breaking the existing sites, I could just add a check in the new if, checking if the attr exists.

@yousinix
Copy link
Owner

My bad, didn't understand it that way.
For the other approach you are proposing, I think it would be better if the showPercentage is a site-level configuration in _config.yml for consistency.

@LaQuay
Copy link
Author

LaQuay commented Oct 24, 2021

Hi, sorry for the late reply. I did not get the notification.

With your proposal, you cannot decide whether you want for some and not for others. There are different sections and with the solution, I propose you can do several things. With your proposal, it is either YES or NO.

@yousinix
Copy link
Owner

I don't understand why we might need to hide the percentage label for some skills and not for others. This behavior seems inconsistent.

@sakgoyal
Copy link

sakgoyal commented Aug 7, 2022

you can instead just do something like {% unless hidePercentage == true %} so it doesnt break existing pages right? this way it will only hide if the variable is defined and set to true. if its not defined, nothing will change.

Edit: like this here.

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