-
-
Notifications
You must be signed in to change notification settings - Fork 161
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
Lost center fraction #365
Lost center fraction #365
Conversation
Phew...sorry I've been silent. Life apparently? 🕺 I only have a few minutes to review this–so I'm gonna leave some inline comments...mainly for myself to double check my own assumptions. Event though I wrote the code sometimes I forget how things work. Initial glance has me super stoked to have you helping out! It looks awesome! I should have some time to do a deep dive on Thursday evening Pacific time. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All of this looks awesome! ...my only reason for not merging right away is because I want to fully grasp everything in here not because I don't think it's great.
...as I said in the PR, will hopefully get to this Thursday evening.
/** | ||
* lost-center: Horizontally center a container element and apply padding | ||
* to it. | ||
* | ||
* @param {length} [max-width] - A max-width to assign. Can be any unit. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@codebysubtract With these edits here, do you have any thoughts on: #364
lostCenterFlexbox = declaration.value; | ||
} | ||
if (!lgLogic.validateUnit(lostUnit, validUnits)) { | ||
decl.warn(result, `${lostUnit} is not a valid unit for lost-center`); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@codebysubtract 🕺 🎆
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These look awesome! Thanks for putting in the time to make this awesome!
What kind of change is this? (Bug Fix, Feature...)
Feature Enhancement
What is the current behavior (You can also link to an issue)
Allow for fractions to be used within lost-center #361
What is the new behavior this introduces (if any)
Being able to use a fraction w/in lost-center
Does this introduce any breaking changes?
No
Does the PR fulfill these requirements?
Other Comments