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

Should the iff function be public or namespaced? #1735

Closed
NickColley opened this issue Feb 12, 2020 · 4 comments
Closed

Should the iff function be public or namespaced? #1735

NickColley opened this issue Feb 12, 2020 · 4 comments

Comments

@NickColley
Copy link
Contributor

NickColley commented Feb 12, 2020

Something I noticed when doing the work to show the sass docs is that the iff function is the odd one out. It is public but does not have govuk- namespace.
Proposals

  1. make it private by changing it's name to _iff and updating sass docs
  2. make it consistent by changing it's name to govuk-iff

My preference would be to make it private since it's not a GOV.UK Frontend specific thing and is really an internal function.

@NickColley NickColley added this to the v4.0.0 milestone Feb 12, 2020
@NickColley NickColley added the awaiting triage Needs triaging by team label Feb 12, 2020
@36degrees
Copy link
Contributor

We could also consider removing if and just using Sass' built-in if function – I don't think it'd make sense to rename it to govuk-iff as we lose all the benefits of conciseness over if.

Have we seen any evidence of this causing issues for users?

@36degrees 36degrees added breaking change 🕔 hours A well understood issue which we expect to take less than a day to resolve. Priority: low and removed awaiting triage Needs triaging by team labels Feb 17, 2020
@NickColley
Copy link
Contributor Author

NickColley commented Feb 19, 2020

Have we seen any evidence of this causing issues for users?

No evidence other than when generating the Sass docs it creates an inconsistency and we'd then be promoting something that feels like we made a mistake on.

I also think there's likely not much use of this function yet so would prefer sort it out before having it document, or as my preference would be to make it private.

We could make it private without changing the name of course.

We could also consider removing if and just using Sass' built-in if function

Would be up for avoiding it entirely if that's an option.

@36degrees
Copy link
Contributor

For context, this was added as 'syntactic sugar' for situations like this:

@mixin govuk-typography-weight-regular($important: false) {
  font-weight: $govuk-font-weight-regular iff($important, !important);
}

Sass' default if function always requires you to pass an else clause:

.foo {
  font-weight: $govuk-font-weight-regular if($important, !important, ""); 
}

NickColley added a commit that referenced this issue Feb 21, 2020
- make the function a private function to indicate it should not be
used.
- add warning and deprecation sassdoc to indicate it will be removed in
the future.

#1735
@36degrees
Copy link
Contributor

Closing in favour of #1771 as we've made a decision and I believe the original issue has been resolved.

@36degrees 36degrees removed breaking change 🕔 hours A well understood issue which we expect to take less than a day to resolve. labels Mar 27, 2020
@36degrees 36degrees removed this from the v4.0.0 milestone May 18, 2020
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

No branches or pull requests

2 participants