Stacks icon indicating copy to clipboard operation
Stacks copied to clipboard

refactor(post-summary): use pseudo-private custom properties

Open dancormier opened this issue 3 years ago β€’ 7 comments

dancormier avatar Sep 02 '22 17:09 dancormier

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...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

netlify[bot] avatar Sep 02 '22 17:09 netlify[bot]

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...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

netlify[bot] avatar Sep 02 '22 17:09 netlify[bot]

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.

giamir avatar Oct 10 '22 09:10 giamir

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.

ondrejkonec avatar Oct 10 '22 13:10 ondrejkonec

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! πŸ’›

dancormier avatar Oct 11 '22 16:10 dancormier

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. 😊

giamir avatar Oct 11 '22 17:10 giamir

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-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?

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 😝

dancormier avatar Oct 11 '22 17:10 dancormier

@giamir odd! Thanks for looking at this branch. Were there any merge conflicts when merging in the latest develop?

dancormier avatar Jan 04 '23 15:01 dancormier

@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.

giamir avatar Jan 04 '23 15:01 giamir

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 🀞

dancormier avatar Jan 04 '23 23:01 dancormier