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(content): add fixedSlotPlacement prop #29233

Merged
merged 7 commits into from
Apr 2, 2024
Merged

feat(content): add fixedSlotPlacement prop #29233

merged 7 commits into from
Apr 2, 2024

Conversation

mapsandapps
Copy link
Contributor

@mapsandapps mapsandapps commented Mar 28, 2024

Issue number: Internal


What is the current behavior?

Content in the fixed slot is always placed after the main content in the DOM.

What is the new behavior?

  • A new fixedSlotPlacement prop on Content allows developers to place fixed content either before or after the main content in the DOM

Does this introduce a breaking change?

  • Yes
  • No

Other information

Dev build: 8.0.0-dev.11712072527.1dd97c66

⚠️This feature will not be part of the v8.0 release. As a result, do not merge this into feature-8.0. However, I am putting this PR up based off feature-8.0 so it can get reviewed by the team.

@github-actions github-actions bot added the package: core @ionic/core package label Mar 28, 2024
* For example, if a FAB in the fixed slot should receive keyboard focus before
* the main page content, set this property to `'beforeContent'`.
*/
@Prop() fixedSlotPlacement: 'afterContent' | 'beforeContent' = AFTER_CONTENT_SLOT_POSITION;
Copy link
Contributor

Choose a reason for hiding this comment

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

Bikeshedding: Thoughts on the value names?

I think fixedSlotPlacement makes sense here. We use labelPlacement in forms control to describe where the visible text is placed relative to the form control in the DOM, so this naming is consistent with other APIs.

For the values we tend to use kebab case, so before-content and after-content could make sense. However, I could be convinced to just call it before and after. The prop description already describes that this prop controls the slot positioning relative to the content, so I don't know how much devs get by having to repeat the content terminology.

Copy link
Member

Choose a reason for hiding this comment

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

I agree that we don't typically use camelCase when writing values for strings. For example:

// datetime.tsx

@Prop() presentation: DatetimePresentation = 'date-time';
// segment-button.tsx

@Prop() layout?: SegmentButtonLayout = 'icon-top';

I also don't see any benefit to including the word content as this is really placing it before or after the scroll element. I vote we change this to before and after.

@liamdebeasi liamdebeasi marked this pull request as ready for review April 1, 2024 18:59
@liamdebeasi liamdebeasi changed the base branch from feature-8.0 to feature-8.1 April 2, 2024 14:33
@liamdebeasi liamdebeasi merged commit 8905696 into feature-8.1 Apr 2, 2024
52 checks passed
@liamdebeasi liamdebeasi deleted the FW-2070 branch April 2, 2024 15:52
liamdebeasi added a commit that referenced this pull request Apr 3, 2024
Issue number: Internal

---------

<!-- Please do not submit updates to dependencies unless it fixes an
issue. -->

<!-- Please try to limit your pull request to one type (bugfix, feature,
etc). Submit multiple pull requests if needed. -->

## What is the current behavior?
<!-- Please describe the current behavior that you are modifying. -->
Content in the `fixed` slot is always placed after the main content in
the DOM.

## What is the new behavior?
<!-- Please describe the behavior or changes that are being added by
this PR. -->

- A new `fixedSlotPlacement` prop on Content allows developers to place
fixed content either before or after the main content in the DOM

## Does this introduce a breaking change?

- [ ] Yes
- [x] No

<!--
  If this introduces a breaking change:
1. Describe the impact and migration path for existing applications
below.
  2. Update the BREAKING.md file with the breaking change.
3. Add "BREAKING CHANGE: [...]" to the commit description when merging.
See
https://github.com/ionic-team/ionic-framework/blob/main/.github/CONTRIBUTING.md#footer
for more information.
-->


## Other information

Dev build: `8.0.0-dev.11712072527.1dd97c66`

<!-- Any other information that is important to this PR such as
screenshots of how the component looks before and after the change. -->

⚠️This feature will not be part of the v8.0 release. As a result, do not
merge this into `feature-8.0`. However, I am putting this PR up based
off `feature-8.0` so it can get reviewed by the team.

---------

Co-authored-by: Liam DeBeasi <liamdebeasi@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package: angular @ionic/angular package package: core @ionic/core package package: vue @ionic/vue package
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants