Skip to content

feat(tab-bar): make box shadow independent of expand mode #30117

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 3 commits into from
Jan 13, 2025
Merged

feat(tab-bar): make box shadow independent of expand mode #30117

merged 3 commits into from
Jan 13, 2025

Conversation

@OS-pedrolourenco
Copy link
Contributor

@OS-pedrolourenco OS-pedrolourenco commented Jan 8, 2025 •
edited
Loading

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

vercel bot commented Jan 8, 2025 •
edited
Loading

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
thetaPC
thetaPC requested changes Jan 13, 2025
View reviewed changes
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
Contributor Author

OS-pedrolourenco 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.

@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 •
edited
Loading

@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.

thetaPC
thetaPC approved these changes Jan 13, 2025
View reviewed changes
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 OS-pedrolourenco mentioned this pull request Jan 14, 2025
Merged
2 tasks
OS-pedrolourenco added a commit that referenced this pull request Jan 17, 2025
@OS-pedrolourenco @thetaPC
test(tab-bar): add snapshot for box shadow (#30134)
b3e869e 
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 <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@thetaPC thetaPC thetaPC approved these changes

@brandyscarney brandyscarney Awaiting requested review from brandyscarney

Assignees

No one assigned

Labels

package: core @ionic/core package

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@OS-pedrolourenco @thetaPC