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

Flattened prop syntax for parts of prop DSL #39

Closed
cmeeren opened this issue Jan 25, 2020 · 4 comments
Closed

Flattened prop syntax for parts of prop DSL #39

cmeeren opened this issue Jan 25, 2020 · 4 comments
Labels
help wanted Extra attention is needed

Comments

@cmeeren
Copy link
Collaborator

cmeeren commented Jan 25, 2020

In the new theme creation DSL (#2) the theme props are defined in a flat list. This could be used for some component props as well.

Please let me know what you think.

To support this, I will have to run unflatten (from flat) for the props of every component, but according to @Zaid-Ajaj in #2 (comment) it shouldn't be a problem.

It will also require some changes to the generator to support this for non-classes props. Not sure how significant these will be. But there seems to be only two such props anyway, so it might not be worth it (given limited time and resources).

Candidates:

classes and anything ending with Classes (e.g. link.TypographyClasses)

Current:

link.classes [
  classes.link.root "foo"
  classes.link.button "bar"
]
link.typographyClasses [
  classes.typography.root "foo"
  classes.typography.body1 "bar"
]

Proposed:

link.classes.root "foo"
link.classes.button "bar"
link.typographyClasses.root "foo"
link.typographyClasses.body1 "bar"

popover.anchorOrigin and popover.transformOrigin

Current:

popover.anchorOrigin(horizontal = 20, vertical = PopoverOriginVertical.Center)

Proposed:

popover.anchorOrigin.horizontal 20
popover.anchorOrigin.vertical.center
@Zaid-Ajaj
Copy link

I am entirely familiar with the way the classes API is used within Mui but judging from the current vs. proposed snippets, I would definitely go for the latter ❤️

@cmeeren cmeeren added the help wanted Extra attention is needed label Jan 25, 2020
@cmeeren
Copy link
Collaborator Author

cmeeren commented Jan 25, 2020

Hm, I'm having some trouble using unflatten in createElement...

I have this at the top of my components file:

let reactElement (el: ReactElementType) (props: 'a) : ReactElement =
  import "createElement" "react"

let createElement (el: ReactElementType) (properties: IReactProperty seq) : ReactElement =
  reactElement el (createObj !!properties |> Flat.flat.unflatten))

This works fine for some prop lists, such as this:

image

However, for others, such as this, it enters into an infinite recursion:

image

I have no idea what's happening or how to fix it.

Reported here: hughsk/flat#94

@Zaid-Ajaj

This comment has been minimized.

cmeeren added a commit that referenced this issue Feb 5, 2020
@cmeeren
Copy link
Collaborator Author

cmeeren commented Feb 5, 2020

Flattened classes props as of 0.13.0. I have decided not to bother with popover.anchorOrigin and popover.transformOrigin until someone actually requests it. Doing it "properly" requires nontrivial changes to the generator, and doing it by hand increases the maintenance burden if those props are ever updated. Besides, they already have easily accessible "enum" values for topLeft, bottomCenter etc.

@cmeeren cmeeren closed this as completed Feb 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

2 participants