bloom-housing / ui-seeds

Bloom's affordable housing design system
Apache License 2.0
1 stars 1 forks source link

feat: new FieldValue component #14

Closed jaredcwhite closed 1 year ago

jaredcwhite commented 1 year ago

Addresses #6

A couple of observations:

Because we're using semantic text tokens from the design system, I didn't add any FieldValue-specific theme variables, because I imagine we'd just want FieldValue to pick up any global overrides of the semantic tokens if present. But let me know if we should revisit that.

Continuing in the data-* vein, I took a conceptual page from the world of web components/shadow parts, and styled the subcomponents using data-part rather than something BEM-ish like field-value__label, field-value__value, etc. Let me know what you think of that sort of approach, rather than using class names.

netlify[bot] commented 1 year ago

Deploy Preview for storybook-ui-seeds ready!

Name Link
Latest commit b8351c940234d254ad8b2152628433b78d803725
Latest deploy log https://app.netlify.com/sites/storybook-ui-seeds/deploys/6411ffb60da4910008d371f9
Deploy Preview https://deploy-preview-14--storybook-ui-seeds.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.

jaredcwhite commented 1 year ago

@slowbot I wanted to double-check with you the text colors I'm using:

to produce:

image

emilyjablonski commented 1 year ago

We can chat about this in our meeting this afternoon, but I think I would expect to see component level CSS overrides available. I just did the work to align this component with Detroit, and there are a few variables Detroit needs to customize (https://github.com/bloom-housing/ui-components/pull/45/files) We can see if the global tokens might encompass what we need.

jaredcwhite commented 1 year ago

@slowbot I believe I've gotten the right spacing/colors set now, so thanks for the updated Figma docs, and @emilyjablonski I added theme variables for all the component rules. Let me know what you think!

slowbot commented 1 year ago

@jaredcwhite Design wise I think this looks great.

I question the use of data attributes for css classes. I would just want to make sure the approach is done consistently across components and that other contributing engineers can extend this way of styling.

emilyjablonski commented 1 year ago

@slowbot @jaredcwhite Thoughts about chatting about it as a pattern briefly on Thursday?

jaredcwhite commented 1 year ago

@slowbot @emilyjablonski sure, sounds good

github-actions[bot] commented 1 year ago

:tada: This PR is included in version 1.2.0 :tada:

The release is available on:

Your semantic-release bot :package::rocket:

slowbot commented 1 year ago

1DF5B168-E9C5-4298-A36B-C2D3568C2B48_1_105_c.jpeg

We can always push this a bug ticket @jaredcwhite

jaredcwhite commented 1 year ago

@slowbot Yeah, if it seems to happen only in the zeroheight iframe, maybe we should bring that up with them.