Skip to content

Conversation

corollari
Copy link
Contributor

@corollari corollari 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 @ionic/core package label Jun 8, 2019
@liamdebeasi
Copy link
Contributor

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
Copy link
Contributor Author

corollari commented Jun 10, 2019

Edit: Nevermind, just solved my question.

@corollari
Copy link
Contributor Author

@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 brandyscarney changed the title Fix #18484 feat(item-divider): add inner padding CSS variables Jun 10, 2019
@brandyscarney
Copy link
Member

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 June 10, 2019 19:03
@corollari
Copy link
Contributor Author

@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
Copy link
Member

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

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

Thanks again! 🙂

@corollari
Copy link
Contributor Author

Kudos for the awesome handling of this PR 😄

abennouna pushed 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

Labels

package: core @ionic/core package

Projects

None yet

Development

Successfully merging this pull request may close these issues.

bug: ion-item-divider (iOS) has no "--padding-inline-end" variable

3 participants