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

Lost center fraction #365

Merged
merged 3 commits into from
Apr 7, 2017
Merged

Lost center fraction #365

merged 3 commits into from
Apr 7, 2017

Conversation

steve-holland
Copy link
Contributor

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?

  • [✅] Tests for the changes have been added
  • [✅] Docs have been added or updated

Other Comments

@peterramsing
Copy link
Owner

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.

Copy link
Owner

@peterramsing peterramsing left a 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.
Copy link
Owner

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`);
Copy link
Owner

Choose a reason for hiding this comment

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

@codebysubtract 🕺 🎆

Copy link
Owner

@peterramsing peterramsing left a 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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants