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

Combined keywords #22

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Combined keywords #22

wants to merge 5 commits into from

Conversation

frenic
Copy link
Owner

@frenic frenic commented Apr 4, 2018

Combined literals would expand many properties and get rid of the | string. I did some smaller try outs but I realized that the syntax parser needs to group by combinator precedence for this to become less complicated. The real challenge after that is to combine different combinators with different multipliers. But I would be happy with just something to start with as long as nothing but | string is removed and no literals as of today.

  • Add tests
  • Group syntax entities by combinator precedence
  • Primary goal: Get rid of | string from display
  • Secondary goal: Get rid of | sting from align-content

Fixes #8

@frenic frenic force-pushed the combined-keywords branch 4 times, most recently from db9f7a8 to 80512fe Compare April 5, 2018 11:31
@pelotom
Copy link
Collaborator

pelotom commented Apr 5, 2018

It looks like align-self also needs this treatment?

@frenic
Copy link
Owner Author

frenic commented Apr 5, 2018

The syntax is quite alike align-content so I guess it will solve itself when it's done 😃

@frenic frenic force-pushed the combined-keywords branch 2 times, most recently from 054dcce to c89f755 Compare April 6, 2018 11:33
@frenic
Copy link
Owner Author

frenic commented Apr 6, 2018

Syntax entities are now grouped by combinator precedence. The identification whether a component should be included or not is simplified but will likely be revised when component combinations will be implemented.

I'm a bit surprised that this didn't cause any change to the definitions. You know that feeling when you've no clue why it works and you're convinced that you're not doing it completely right. But when you fix it, the output is completely identical. 😆

@pelotom
Copy link
Collaborator

pelotom commented Aug 1, 2018

In case this is proving hard to implement, allow me to propose something simple: I think most people who care about type safety, would be willing to forego the ability to use combined literals in exchange for getting rid of these | string fallbacks. What if there was simply an option to disable them? This could either take the form of a type parameter to all the interfaces, or more simply, a global Options interface which controls this (and maybe other things in the future):

interface Options {
}

this could then be overridden by the user with module augmentation:

declare module 'options' {
  export interface Options {
    allowCombinedKeyword: true;
  }
}

and finally used in a conditional type expression to determine if the | string fallbacks should be used or removed due to the presence of that option field.

@frenic
Copy link
Owner Author

frenic commented Aug 1, 2018

It's a bit complicated to implement, yes. But I've made some progress and I'm planning to resume that work after I my vacation. Hopefully! 😸

That case you're suggesting doesn't make things a lot easier. We would need to be selective and ignore | string for only certain properties. We cannot just remove | string everywhere. To automatically identify these properties would be nearly impossible because we would need some kind of statistics to determine how often combinations are used. Doing it manually ruins the whole idea of everything being automated and would require us to maintain that list depending on global usage over time.

I would rather put my efforts in finishing this. Sorry.

@GabrielCTroia
Copy link

Hey guys, I got hit with this issue too. I’d like my cas to be all types, in a similar fashion as react-native, but I was very surprised to find out this is not the case with the web unforthnately.

What is the status with this PR?

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.

display should not allow an arbitrary string
3 participants