Add error molecule pattern #30
Conversation
|
I have left out the mixins that need the logical properties mixin but have put in a TODO for each one. |
centering was confusing patterns that center themselves ie. error molecule pattern
There was a problem hiding this comment.
Don't think it needs to be addressed now, but noting that we also have an error Sass module in addition to this error pattern. If this causes confusion, I'd prefer to rename this pattern rather than the sass module, as the error.throw()formulation for the sass module works well.
| } | ||
|
|
||
| .error__heading { | ||
| @include space.block($end: rem.calculate(baseline.for-units(2))); |
There was a problem hiding this comment.
(Out of scope.) Wondering if we should have a mixin for rem.calculate(baseline.for-units(2)) as it seems a reasonably common use case.
There was a problem hiding this comment.
Possibly but I always think having mixins within mixins could confuse and how many mixins we want to nest before it's too much?
There was a problem hiding this comment.
My thinking is one well chosen name could make the code easier to read, rather than having to mentally parse the nested call in the style rules on each occurrence 🤔
|
|
||
| .error { | ||
| text-align: center; | ||
| @include size.max-inline-size(grid.$max-inline-size); |
There was a problem hiding this comment.
(Out of scope) @include size.max-inline-size() seems a bit clumsy.
Renaming the dimension constraint mixins could give us either:
1.
@include size.min-inline()@include size.max-inline()
or
@include size.inline-min()@include size.inline-max()
Think I prefer 1 as it more closely matches the underlying css property names. Thoughts? (Would be a separate PR.)
There was a problem hiding this comment.
I think number one is better as it matches CSS styling like max-width or max-height
| @include space.margin(auto, inline); | ||
| @include space.padding(grid.$column_gap, inline); | ||
| @include space.block($start: rem.calculate(baseline.for-units(12)), $end: rem.calculate(baseline.for-units(12))); | ||
| @include media-query.mq($from: medium) { |
There was a problem hiding this comment.
(Out of scope.) While I'm on a roll with the naming revisionism... 😅 I don't much like @include media-query.mq() either. @include media.query() would read better, but not sure how feasible this is with how we're using the mq library. Will have a play and see if can get something sensible from it (into a separate PR).
No description provided.