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(tab-bar): make box shadow independent of expand mode #30117

Merged
merged 3 commits into from
Jan 13, 2025

Conversation

OS-pedrolourenco
Copy link

@OS-pedrolourenco OS-pedrolourenco commented Jan 8, 2025

Issue number: internal

What is the current behavior?

When the tab bar's expand mode is 'full' on the ionic theme, it does not have a box shadow to separate it from the rest of the page.

What is the new behavior?

  • The tab bar on the ionic theme now has a box shadow independently of its expand mode.

Does this introduce a breaking change?

  • Yes
  • No

Other information

@OS-pedrolourenco OS-pedrolourenco requested a review from a team as a code owner January 8, 2025 15:18
Copy link

vercel bot commented Jan 8, 2025

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
ionic-framework ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jan 9, 2025 3:07pm

@github-actions github-actions bot added the package: core @ionic/core package label Jan 8, 2025
@OS-pedrolourenco OS-pedrolourenco changed the title Making tab bar box shadow independent of expand mode feat(tab-bar): making box shadow independent of expand mode Jan 8, 2025
@OS-pedrolourenco OS-pedrolourenco requested review from brandyscarney and removed request for thetaPC January 8, 2025 15:19
@thetaPC
Copy link
Contributor

thetaPC commented Jan 8, 2025

@OS-pedrolourenco please make sure the PR checks are passing before marking it ready for review. It speeds up the process for the reviewer.

@OS-pedrolourenco OS-pedrolourenco marked this pull request as draft January 8, 2025 16:12
@OS-pedrolourenco OS-pedrolourenco marked this pull request as ready for review January 9, 2025 14:54
Copy link
Contributor

@thetaPC thetaPC left a comment

Choose a reason for hiding this comment

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

One small nitpick, the PR title. This is not a feat, a feat is when we're adding something new. In this case, we're making a change to an existing style, so it's a refactor. Also we try to start the sentence with a verb.

refactor(tab-bar): make box shadow independent of expand for ionic

@OS-pedrolourenco
Copy link
Author

@thetaPC I have changed the behaviour of the tab-bar so that it now has a shadow when its expand mode is 'full' and not just 'compact', as requested by UX. Wouldn't you say that that is feature and not a refactor?
I will correct the verb usage right away though. Just wanted to align with you on this for future reference.

@OS-pedrolourenco OS-pedrolourenco changed the title feat(tab-bar): making box shadow independent of expand mode feat(tab-bar): make box shadow independent of expand mode Jan 13, 2025
@thetaPC
Copy link
Contributor

thetaPC commented Jan 13, 2025

@thetaPC I have changed the behaviour of the tab-bar so that it now has a shadow when its expand mode is 'full' and not just 'compact', as requested by UX. Wouldn't you say that that is feature and not a refactor? I will correct the verb usage right away though. Just wanted to align with you on this for future reference.

As you mentioned, you did a code change so that's a refactor. Here's the definitions for:

  • refactor: A code change that neither fixes a bug nor adds a feature
  • feat: A new feature
    More information on these can be found at the Conventional Commits site.

Edit:
After some thought, I can see a case for either feat or refactor. So I will leave it up to you.

Copy link
Contributor

@thetaPC thetaPC left a comment

Choose a reason for hiding this comment

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

LGTM

@OS-pedrolourenco OS-pedrolourenco merged commit b0c5555 into next Jan 13, 2025
49 checks passed
@OS-pedrolourenco OS-pedrolourenco deleted the ROU-11503 branch January 13, 2025 18:55
OS-pedrolourenco added a commit that referenced this pull request Jan 17, 2025
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. -->
This PR is a follow-up of this one:
#30117

## What is the new behavior?
<!-- Please describe the behavior or changes that are being added by
this PR. -->
- This PR adds a snapshot test and modifies the existing one in order to
test the change done in the previous PR.

## 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/docs/CONTRIBUTING.md#footer
for more information.
-->


## Other information

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

---------

Co-authored-by: Maria Hutt <maria@ionic.io>
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.

2 participants