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

fix(item): remove empty padding space for item bottom #24323

Merged
merged 3 commits into from
Dec 8, 2021
Merged

Conversation

sean-perkins
Copy link
Contributor

@sean-perkins sean-perkins commented Dec 6, 2021

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?

The .item-bottom implementation for ion-item, incorrectly pushes the item's contents regardless if help/error text is present.

Screen Shot 2021-12-06 at 3 45 40 PM

Issue Number: Resolves #23892

What is the new behavior?

The reserved space for the help/error text will not push the container space when the slot is unused.

Screen Shot 2021-12-06 at 3 39 34 PM

Does this introduce a breaking change?

  • Yes
  • No

Other information

Changes for help/error text and the item bottom container were introduced with: #23354

@github-actions github-actions bot added the package: core @ionic/core package label Dec 6, 2021
Copy link
Contributor

@liamdebeasi liamdebeasi left a comment

Choose a reason for hiding this comment

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

This is a great start! Can we also add a screenshot test so this does not regress?

core/src/components/item/item.md.scss Outdated Show resolved Hide resolved
core/api.txt Outdated Show resolved Hide resolved
core/src/components/item/item.scss Outdated Show resolved Hide resolved
core/src/components/item/item.tsx Outdated Show resolved Hide resolved
@sean-perkins sean-perkins removed the request for review from averyjohnston December 6, 2021 21:57
@sean-perkins sean-perkins changed the title fix(item): padding for MD spec and counter appearance fix(item): remove empty padding space for item bottom Dec 7, 2021
0,
var(--inner-padding-end),
0,
calc(var(--padding-start) + var(--ion-safe-area-left, 0px) + var(--bottom-padding-start))
Copy link
Contributor

Choose a reason for hiding this comment

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

--bottom-padding-start is not part of the public API, and defaults to 0px, so do we even need to use it anymore?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd feel comfortable removing it. Especially as another draft PR exposes .item-bottom as a shadow part that would allow developers to customize these styles in 6.1.

Is our stance that anything documented is public API and any other CSS variables are internal use and lower concern of affecting our release cycle?

Copy link
Contributor

@liamdebeasi liamdebeasi Dec 7, 2021

Choose a reason for hiding this comment

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

Yep exactly. If it's not in the comments above in that item.scss file (this causes stencil to autogenerate it in the docs.json file) then it's internal.

Copy link
Contributor

@liamdebeasi liamdebeasi left a comment

Choose a reason for hiding this comment

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

Great job!

@ashish-pio
Copy link

Is this merged into @ionic/vue? I am using the vue implementation of ionic, v6.1.11 but yet facing the empty space from .item- bottom class

@averyjohnston
Copy link
Contributor

This was released in Ionic 6.0.1: https://github.com/ionic-team/ionic-framework/releases/tag/v6.0.1 Could you create a separate issue with a minimal reproduction?

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.

4 participants