Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add styles typing for consumers #88

Merged
merged 3 commits into from Aug 21, 2020
Merged

Add styles typing for consumers #88

merged 3 commits into from Aug 21, 2020

Conversation

jjenzz
Copy link
Contributor

@jjenzz jjenzz commented Aug 10, 2020

Closes #48

This solution isn't ideal because if a component name changes, we'll have a mismatch between the style object key and the typing if the developer forgets to update the PrimitiveStyles generic.

Unfortunately, my research into how we could handle this a bit better lead me to these two TS tickets which aren't currently possible:

The alternatives I explored all seemed to create too many complex abstractions which became hard to follow. I wanted to get this PR up so it at least works for release and then we can discuss how to solve this in a less human error prone way. It may be okay as is though since these values shouldn't change very often.

Any ideas? Solved

@jjenzz jjenzz changed the title Add typing for consumers Add styles typing for consumers Aug 10, 2020
@benoitgrelard
Copy link
Collaborator

@jjenzz what do consumers practically get from this?

@jjenzz
Copy link
Contributor Author

jjenzz commented Aug 11, 2020

@benoitgrelard sorry, should have added this to description:

Before:

pr

After:

pr

@benoitgrelard
Copy link
Collaborator

Right I see, is that because we are expecting users to have to add the styles to the correct parts themselves?

@jjenzz
Copy link
Contributor Author

jjenzz commented Aug 12, 2020

Yep, or use our extracted css file. The options are as follows:

Option one

import { Button as ButtonPrimitive } from '@interop-ui/react-button';
import '@interop-ui/react-button/styles.css';
 
export const Button = styled(ButtonPrimitive, {
  borderRadius: '3px',
  padding: '10px 20px',
});

We have a yarn build:styles script that runs during build to create the style.css file from the primitive styles object. That's why the styles object keys aren't static (and need manually typing) because it switches what the keys output during static extraction process.

Option two

import { Button as ButtonPrimitive, styles } from '@interop-ui/react-button';
 
export const Button = styled(ButtonPrimitive, {
  ...styles.button,
  borderRadius: '3px',
  padding: '10px 20px',
});

@jjenzz
Copy link
Contributor Author

jjenzz commented Aug 12, 2020

I think we can probably refactor to something like:

const Accordion = props => {
	return <div {...interopDataAttrObj<typeof styles>('accordion')} {...props} />
};

export const styles = {
	accordion: {}
	button: {}
	panel: {}
};

and then the extraction script can use interopDataAttrSelector to map over and convert them. I probably should have done this really from the offset but didn't think of it at the time 😬

I might go through and do that instead. Will be a whole bunch of changes but will be cleaner.

@jjenzz jjenzz force-pushed the style-typing branch 2 times, most recently from e6d548a to cfdd816 Compare August 12, 2020 17:42
@@ -290,8 +283,8 @@ interface RadioStaticProps {
Icon: typeof RadioIcon;
}

const styles: PrimitiveStyles = {
[interopSelector(ROOT_NAME)]: {
const [styles, interopDataAttrObj] = createStyleObj(RADIO_NAME, (selector) => ({
Copy link
Contributor Author

@jjenzz jjenzz Aug 12, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one is slightly different and uses a function callback so that we can get the right namespaced selector if we need to reference it in other component styles.

Copy link
Contributor Author

@jjenzz jjenzz Aug 18, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've removed the need for this in a more recent PR #121

export function createStyleObj<Part extends string = string>(
namespace: string,
originalStyles:
| ((selector: (part: string) => Selector) => PrimitiveStyles<Part>)
Copy link
Contributor Author

@jjenzz jjenzz Aug 12, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't make this part: Part here as it becomes part: never because inference fails

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've removed the need for this in a more recent PR #121

@jjenzz
Copy link
Contributor Author

jjenzz commented Aug 12, 2020

@benoitgrelard @chancestrickland Okay, this is huge now but it's ready. 😬

@@ -310,11 +303,11 @@ const styles: PrimitiveStyles = {
zIndex: 1,
opacity: 0,

[`&:checked + ${interopDataAttrSelector(ICON_NAME)}`]: {
[`&:checked + ${selector('icon')}`]: {
Copy link
Contributor Author

@jjenzz jjenzz Aug 12, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Annoyingly the icon string isn't typed here, it just expects string. It started to take a while to figure out and since it's only needed in one component, I time-boxed it and failed haha. I'd be surprised if it's possible tbh.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've remove the need for this in a more recent PR #121

@chaance
Copy link
Contributor

chaance commented Aug 13, 2020

Didn't see any issues at first glance, and typing seems to work as expected. Ran out of time for a thorough review of this one but I may try to look again this weekend if you don't merge before then!

@jjenzz
Copy link
Contributor Author

jjenzz commented Aug 21, 2020

@chancestrickland @benoitgrelard I'm going to get this in so I can progress in other areas in your absence. Feel free to raise stuff after merge and I'll address 🙂

@jjenzz jjenzz merged commit c23e87b into main Aug 21, 2020
@jjenzz jjenzz deleted the style-typing branch August 21, 2020 15:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fix typing for exported styles now that we have dynamic style keys
3 participants