refactor(post-summary): use pseudo-private custom properties
Deploy Preview for stacks ready!
| Name | Link |
|---|---|
| Latest commit | 6f4c16a6e71796cfc3ab2d32f5b51d110d19ca67 |
| Latest deploy log | https://app.netlify.com/sites/stacks/deploys/63124290a6d3bb00098bb87d |
| Deploy Preview | https://deploy-preview-1091--stacks.netlify.app |
| Preview on mobile | Toggle QR Code...Use your smartphone camera to open QR code link. |
To edit notification comments on pull requests, go to your Netlify site settings.
Deploy Preview for stacks ready!
| Name | Link |
|---|---|
| Latest commit | 96c40da174333bbc85f6080a7162428a8b4ab658 |
| Latest deploy log | https://app.netlify.com/sites/stacks/deploys/63b605ed72bd98000890a39a |
| Deploy Preview | https://deploy-preview-1091--stacks.netlify.app |
| Preview on mobile | Toggle QR Code...Use your smartphone camera to open QR code link. |
To edit notification comments on pull requests, go to your Netlify site settings.
Hello @dancormier and @b-kelly, I spent some time doing a high level review of this PR (with the limited context I have).
Here are some comments/questions/ideas:
- What do we hope to achieve by introducing "pseudo-private custom properties"? My guess here is that we hope they will make the css codebase more readable/maintainable. Are there any other reasons I miss (e.g. do we expect consumers to leverage the dynamic nature of CSS custom properties - changing values at runtime)?
- If the main goal is to make things more readable, maintainable and organised I have 2 suggestions:
- If we don't need any dynamic behavior from these properties, what about converting them to preprocessor static variables instead (as recommended here)? Such approach would also prevent consumers to change the value of the properties at runtime reinforcing their "private" nature
- What would you think about declaring all the variables at the top of the CSS file (similarly to what the author describes here as the logic fold)? This might establish a stronger pattern to recognise across the codebase (all the files starting with variable declarations).
- About the naming "pseudo-private custom properties" - what "pseudo" represents? If my thinking above is correct and there are no other reasons to introduce those variables apart from maintainability what about calling them just "local variables"?
- Is there already a way in Stacks to keep track about why certain architecture decisions were taken? If not, it could be beneficial to start lightweight architecture decision records. I have used similar tools in the past: they are relatively inexpensive and they hold important context for new contributors but also for established ones. I'd be happy to give an introduction to ADRs if you don't know them and are interested.
As you can see I have more questions than answers but I hope this input is useful anyway. π Happy to talk more sync about this topic.
Hello @dancormier and @b-kelly, I spent some time doing a high level review of this PR (with the limited context I have).
Here are some comments/questions/ideas:
What do we hope to achieve by introducing "pseudo-private custom properties"? My guess here is that we hope they will make the css codebase more readable/maintainable. Are there any other reasons I miss (e.g. do we expect consumers to leverage the dynamic nature of CSS custom properties - changing values at runtime)?
If the main goal is to make things more readable, maintainable and organised I have 2 suggestions:
- If we don't need any dynamic behavior from these properties, what about converting them to preprocessor static variables instead (as recommended here)? Such approach would also prevent consumers to change the value of the properties at runtime reinforcing their "private" nature
- What would you think about declaring all the variables at the top of the CSS file (similarly to what the author describes here as the logic fold)? This might establish a stronger pattern to recognise across the codebase (all the files starting with variable declarations).
About the naming "pseudo-private custom properties" - what "pseudo" represents? If my thinking above is correct and there are no other reasons to introduce those variables apart from maintainability what about calling them just "local variables"?
Is there already a way in Stacks to keep track about why certain architecture decisions were taken? If not, it could be beneficial to start lightweight architecture decision records. I have used similar tools in the past: they are relatively inexpensive and they hold important context for new contributors but also for established ones. I'd be happy to give an introduction to ADRs if you don't know them and are interested.
As you can see I have more questions than answers but I hope this input is useful anyway. π Happy to talk more sync about this topic.
Hi guys,
I was trying to fix few bugs here as I wanted to help you, but I'm often fighting with CSS architectural problems and poor component structure. I would like to support idea with making this design manipulation more simple, clear and straightforward.
Do you think I could try to suggest some improvements? I'm happy to get involved, but right now I find working with CSS in this code little bit inflexible and unscalable.
Note For brevity, I'll refer to "pseudo-private custom properties" as PPCP
What do we hope to achieve by introducing "pseudo-private custom properties"? My guess here is that we hope they will make the css codebase more readable/maintainable. Are there any other reasons I miss (e.g. do we expect consumers to leverage the dynamic nature of CSS custom properties - changing values at runtime)?
There's a few potential benefits to introducing PPCP, IMO:
- Flattening the cascade. With PPCP, we (mostly) only change custom property value at the base component level, so we can change values without having to untangle complex specificity. Long term, I think this could be useful to alter styles for nested components based on context (though it would kinda undermine the whole "private" aspect to PPCP).
- Improved readability. With PPCP, we explicit call out of properties changed by variants/modifiers/themes/etc and, since a given variant is only changing those properties, it's arguably easier to skim and see what's changing without needing to parse through a cascade of rules.
- Lighter-weight CSS. Because we're changing custom properties at the top level, we don't need to repeat the entire structure of a rule inside a given variant/modifier. We create the structure for the CSS once on the component, which makes it more maintainable and lighten the CSS output for more complex components.
- Reducing our dependency on Less. With CSS nesting and better approaches to colors in native CSS on the horizon, I'm anticipating a day we no longer need a preprocessor. I figure this would speed up development and eliminate the need for contributors to understand CSS and preprocessor syntax. This one is far off in the future, though.
Some of these benefits are personal preference and up for debate, and some benefits don't materialize in every context, but I've found it to be a useful approach on the whole. For further information on PPCP, I recommend Lea Verou's great CSS Variable Secrets talk, which has been the inspiration for this approach.
If we don't need any dynamic behavior from these properties, what about converting them to preprocessor static variables instead (as recommended here)? Such approach would also prevent consumers to change the value of the properties at runtime reinforcing their "private" nature
I'd like to tend toward native CSS and reduce Stacks' dependency on a preprocessor . I think the "pseudo" part of PPCP stands out, but I don't personally see a problem with the properties in question not truly being private.
What would you think about declaring all the variables at the top of the CSS file (similarly to what the author describes here as the logic fold)? This might establish a stronger pattern to recognise across the codebase (all the files starting with variable declarations).
I might be misunderstanding, but I think that's the approach I've taken. Each component's PPCP are declaired at the top within the context of the given component. See _styles-template.less for more details. And let me know if I'm misunderstanding!
About the naming "pseudo-private custom properties" - what "pseudo" represents? If my thinking above is correct and there are no other reasons to introduce those variables apart from maintainability what about calling them just "local variables"?
The naming is just pedantry and I'm not too tied to a specific name, honestly. I adopted the name from Lea Verou. For some details, the "pseudo" refers to the fact that they're not truly private. In CSS, all custom properties are exposed in the output but we treat them as if they're private. And I call them "custom properties" because that's technically their name, but I have not problem with using the term "variables".
Is there already a way in Stacks to keep track about why certain architecture decisions were taken? If not, it could be beneficial to start lightweight architecture decision records. I have used similar tools in the past: they are relatively inexpensive and they hold important context for new contributors but also for established ones. I'd be happy to give an introduction to ADRs if you don't know them and are interested.
I'm a fan of ADRs (in fact, I was just talking to Alex about them last week) and I would love to adopt them. It would probably be a useful exercise to create some while the team grows. We should sync up on this and make some choices about where they should live and what we should document.
As you can see I have more questions than answers but I hope this input is useful anyway. π Happy to talk more sync about this topic.
Keep the questions coming! It's been useful to think through answers and I like having a little documentation to point to when others have the same questions! π
I'd like to tend toward native CSS and reduce Stacks' dependency on a preprocessor . I think the "pseudo" part of PPCP stands out, but I don't personally see a problem with the properties in question not truly being private.
I did not know that we are aiming to stick to native CSS as much as possible. I can certainly get behind that principle though - I like to leverage the platform as much as possible. Can I assume then that we are not supporting IE?
I might be misunderstanding, but I think that's the approach I've taken. Each component's PPCP are declaired at the top within the context of the given component. See _styles-template.less for more details. And let me know if I'm misunderstanding!
In the post-summary file it looks like we are continuing to declare css custom properties (L56) also after some CSS properties have been already defined (L31). I was thinking to take things a step further and take care of all custom properties declarations at the very beginning of the file. In the file you shared that is the case so can I assume that for the post-summary file we are targeting the same but the refactoring is still ongoing?
Thanks a lot for the detailed answer(s) @dancormier. π I really appreciate it and I think the approach makes a lot of sense (so much sense that it might deserve a lightweight ADR). Starting with ADRs doesn't have to be expensive. In one of my previous team we just started by creating an ADRs.md file in the repo and we followed this simple template:
[ADR Title]
Date: [DD.MM.YYYY]
Issue: [describe in few sentences the problem we need to solve]
Status: [ π§ Proposed | β Accepted | π« Rejected ]
Decision: [describe in few sentences the proposed solution to the issue]
I personally like when ADRs are close to the codebase. π
I'd like to tend toward native CSS and reduce Stacks' dependency on a preprocessor . I think the "pseudo" part of PPCP stands out, but I don't personally see a problem with the properties in question not truly being private.
I did not know that we are aiming to stick to native CSS as much as possible. I can certainly get behind that principle though - I like to leverage the platform as much as possible. Can I assume then that we are not supporting IE?
Yes! We are not supporting IE. For a list of browsers we support, see https://browsers.stackoverflow.design/
I might be misunderstanding, but I think that's the approach I've taken. Each component's PPCP are declaired at the top within the context of the given component. See _styles-template.less for more details. And let me know if I'm misunderstanding!
In the
post-summaryfile it looks like we are continuing to declare css custom properties (L56) also after some CSS properties have been already defined (L31). I was thinking to take things a step further and take care of all custom properties declarations at the very beginning of the file. In the file you shared that is the case so can I assume that for thepost-summaryfile we are targeting the same but the refactoring is still ongoing?
Ah, I see! The properties declared on L56 should be defined at the end of the file. Good catch! I've come to define the structure of these component styles more rigidly since creating this PR. I'll make some updates soon to better adhere to the correct structure.
Thanks a lot for the detailed answer(s) @dancormier. π I really appreciate it and I think the approach makes a lot of sense (so much sense that it might deserve a lightweight ADR). Starting with ADRs doesn't have to be expensive. In one of my previous team we just started by creating an ADRs.md file in the repo and we followed this simple template:
[ADR Title]
Date: [DD.MM.YYYY]
Issue: [describe in few sentences the problem we need to solve]
Status: [ π§ Proposed | β Accepted | π« Rejected ]
Decision: [describe in few sentences the proposed solution to the issue]
I personally like when ADRs are close to the codebase. π
Thanks for the template! I'll aim to make a PR for a simple ADR for PPCP this week.
P.S. OMG tech uses a lot of acronyms π
@giamir odd! Thanks for looking at this branch. Were there any merge conflicts when merging in the latest develop?
@dancormier No conflict, but the branch looked pretty outdated so it might be that some other refactor like the gap/gutter one have affected the visuals.
Sorry about the mess @giamir! There were a few rough edges that I've smoothed out. Honestly, it had nothing to do with the age of this branch but was just a few issues I missed in the course of refactoring. It should be good for a review now π€