feat: Add support for empty local variables #854
Conversation
castastrophe
changed the title
…ments into feat-empty-local-variables
castastrophe
added
styles
Co-authored-by: Kendall Totten <[email protected]>
elements/pfe-band/src/pfe-band.scss
Outdated
| // thinner padding on top & bottom | ||
| :host([pfe-size="small"]) { | ||
| --pfe-band--Padding: calc(#{pfe-local(Padding--vertical)} / 4) #{pfe-local(Padding--horizontal)}; | ||
| --pfe-#{$LOCAL}--Padding: calc(#{pfe-local(Padding--vertical)} / 4) #{pfe-local(Padding--horizontal)}; |
There was a problem hiding this comment.
@castastrophe - I see #{$LOCAL} in a bunch of these files instead of using the actual component name. I think this makes reading the code harder and a bit more difficult for debugging issues. If I'm looking the dev tools for and see --pfe-band--Padding and then try to find that variable in the repo as I'm doing development, it's going to be hard to find. Do you think this change is necessary?
There was a problem hiding this comment.
I do - if this is hardcoded and I update the $LOCAL variable at the top, anything using pfe-local function will continue to just work but all of these will break. It might take some work to figure out why all the suddent these variables aren't picking up.
There was a problem hiding this comment.
I recommend using the temp folder with the CSS assets to debug.
There was a problem hiding this comment.
@castastrophe - I don't think we'll ever be changing local though. If we did, it'd be a new component, right?
There was a problem hiding this comment.
Two possible use-cases: You could rename the component variables to add more specificity (nav to navigation or accordion to accordion-header) or you could copy the styles to a new component and want to update it there (without having to grep the entire stylesheet for updates). 🤷
There was a problem hiding this comment.
I like the idea but the likelihood of those use-cases seems infrequent. Weighing this change against the ease of editing the files from day to day, it seems that we'd want to favor the developer experience being easier. I'd like to err on the side of making our code more readable and approachable but if you'd like to solicit feedback from the larger group, I'd be up for that.
There was a problem hiding this comment.
I updated the example to use the pfe-print-local function instead. Same idea but it accepts a map instead. Do you prefer that approach? It has the same benefits without hardcoding values.
Co-authored-by: Kendall Totten <[email protected]>
starryeyez024
left a comment
starryeyez024
left a comment
There was a problem hiding this comment.
Labradors Get The Macaroni!
kylebuch8
left a comment
kylebuch8
left a comment
There was a problem hiding this comment.
I'd like to resolve this conversation before we merge.
#854 (comment)
castastrophe
removed
ready: branch testing
Link(s) to demo pages where these udpates can be viewed (only listed components that have changes):
For component fixes and features
What has changed and why
Testing instructions
Be sure to include detailed instructions on how your update can be tested by another developer.
Browser requirements
Your component should work in all of the following environments:
Ready-for-merge Checklist
Check off items as they are completed. Feel free to delete items if they are not applicable to your PR.
https://rally1.rallydev.com/#/270861059696d/detail/userstory/390981143804