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

feat(item-divider): add inner padding CSS variables #18490

Merged
merged 9 commits into from Jun 10, 2019

Conversation

Projects
None yet
3 participants
@corollari
Copy link
Contributor

commented Jun 8, 2019

Pull request checklist

Please check if your PR fulfills the following requirements:

  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been reviewed and added / updated if needed (for bug fixes / features)
  • Build (npm run build) was run locally and any changes were pushed
  • Lint (npm run lint) has passed locally and any fixes were made for failures

Pull request type

Please check the type of change your PR introduces:

  • Bugfix
  • Feature
  • Code style update (formatting, renaming)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • Documentation content changes
  • Other (please describe):

What is the current behavior?

Issue Number: fixes #18484

What is the new behavior?

It solves: #18484

Does this introduce a breaking change?

  • Yes
  • No

Other information

All credit goes to @brandyscarney, I just implemented the solution she proposed on #18484

@ionitron-bot ionitron-bot bot added the package: core label Jun 8, 2019

@liamdebeasi

This comment has been minimized.

Copy link
Member

commented Jun 10, 2019

Hey there,

Just wanted to mention a couple things before we merge:

  1. Since these are new CSS variables, we need to document them (This will make this PR a feature instead of a bug fix).
  2. The variables need to be initialized before we can use them. You can just initialize them to 0.

Can you make these changes? I can take another look after. Thanks!

@corollari

This comment has been minimized.

Copy link
Contributor Author

commented Jun 10, 2019

Edit: Nevermind, just solved my question.

@corollari

This comment has been minimized.

Copy link
Contributor Author

commented Jun 10, 2019

@liamdebeasi Just applied the changes you requested. Not sure if I should document the variables in a different way, right now it feels very copy-pasty.

brandyscarney added some commits Jun 10, 2019

@brandyscarney brandyscarney changed the title Fix #18484 feat(item-divider): add inner padding CSS variables Jun 10, 2019

@brandyscarney

This comment has been minimized.

Copy link
Member

commented Jun 10, 2019

Thank you @corollari! I added a few small changes. I'll let Liam take another look, but looks good to me! 🙂

@brandyscarney brandyscarney self-requested a review Jun 10, 2019

@corollari

This comment has been minimized.

Copy link
Contributor Author

commented Jun 10, 2019

@brandyscarney cool, it seems I forgot to build the documentation.
Also, given that this is now a new feature, should I change the commit messages? or is that not needed because of the pre-merge squash?

@brandyscarney

This comment has been minimized.

Copy link
Member

commented Jun 10, 2019

@corollari It's not needed, we'll update it on squash & merge. Thank you though!

brandyscarney added some commits Jun 10, 2019

@brandyscarney brandyscarney merged commit 35c143a into ionic-team:master Jun 10, 2019

1 check passed

build Workflow: build
Details
@brandyscarney

This comment has been minimized.

Copy link
Member

commented Jun 10, 2019

Thanks again! 🙂

@corollari

This comment has been minimized.

Copy link
Contributor Author

commented Jun 10, 2019

Kudos for the awesome handling of this PR 😄

abennouna added a commit to abennouna/ionic that referenced this pull request Jun 11, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.