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

Phantom types: add most missing flexbox functions/values #452

Merged
merged 11 commits into from
Apr 7, 2021

Conversation

rlefevre
Copy link

@rlefevre rlefevre commented Jul 26, 2018

Hi, first PR here so feedback is welcome.

Important notes / feedback likely needed

  1. I replaced displayFlex by display flex_ as it seems that underscore won the namespace war for now. Tell me if I'm wrong.
  2. I included a fix to "close" the "open" record parameter of alignItems and all (in passing) as they seemed wrong (their value record was parameterized on accepts). But I may have misunderstood something so tell me if I should remove this modification.
  3. I did not include yet overflow and baseline alignment for justifyContent and alignContent as they are still in a draft specification and are not well supported by all browsers. However alignItems already implements them partially (overflow alignment is limited to safe center or unsafe center but center | start | end | self-start | self-end | flex-start | flex-end should also be valid). So I need feedback to know what to do.
  4. I also did not include intrinsic sizing (min-content, max-content, ...) for flexBasis for the same reasons. However they are listed in the phantom-types issue, so I'm not really sure what we want.

Other less important notes

  • I added a 3rd level of documentation to try to have a nice flexbox section. If this is not wanted, I can flatten out the flexbox documentation to 2 levels as the rest of the document.
  • I renamed flexFlow1 (in the listed function to implement) to flexFlow as it seemed more consistent with other multiple values functions
  • I struggled with the functions definition order. Maybe something should be added in the guidelines about that or a final pass done once all types are implemented.
  • They are some inconsistency between comments about the use of values terminated by an underscore in the source. I choose what seemed the best but a pass to harmonize them is most likely needed at the end.

Thank you very much for this library.
Regards

@rlefevre rlefevre changed the title Phantom flexbox Phantom types: add flexbox functions Jul 26, 2018
@rlefevre rlefevre changed the title Phantom types: add flexbox functions Phantom types: add most missing flexbox functions Jul 26, 2018
@rlefevre rlefevre force-pushed the phantom-flexbox branch 2 times, most recently from 4b6436b to 0cb9289 Compare July 26, 2018 15:14
@rlefevre rlefevre changed the title Phantom types: add most missing flexbox functions Phantom types: add most missing flexbox functions/values Jul 26, 2018
src/Css.elm Outdated
flexWrap noWrap

-}
noWrap : Value { provides | noWrap : Supported }
Copy link
Collaborator

Choose a reason for hiding this comment

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

Awesome work! nowrap is all lowercase as a value. Should we be consistent with the values and make this also all lowercase?

Copy link
Author

@rlefevre rlefevre Jul 27, 2018

Choose a reason for hiding this comment

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

Actually, nowrap already existed for white-space (I missed it), so I removed noWrap and added an example in nowrap.

Thank you!

@rlefevre rlefevre mentioned this pull request Oct 10, 2019
8 tasks
@rtfeldman rtfeldman merged commit e2be6fa into rtfeldman:phantom-types Apr 7, 2021
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.

3 participants