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(banner): add expanded input property to the banner #14935

Merged
merged 18 commits into from
Jan 23, 2025

Conversation

georgianastasov
Copy link
Contributor

Closes #14890

Additional information (check all that apply):

  • Bug fix
  • New functionality
  • Documentation
  • Demos
  • CI/CD

Checklist:

  • All relevant tags have been applied to this PR
  • This PR includes unit tests covering all the new code (test guidelines)
  • This PR includes API docs for newly added methods/properties (api docs guidelines)
  • This PR includes feature/README.MD updates for the feature docs
  • This PR includes general feature table updates in the root README.MD
  • This PR includes CHANGELOG.MD updates for newly added functionality
  • This PR contains breaking changes
  • This PR includes ng update migrations for the breaking changes (migrations guidelines)
  • This PR includes behavioral changes and the feature specification has been updated with them

@georgianastasov georgianastasov added ❌ status: awaiting-test PRs awaiting manual verification banner Banner component version: 18.2.x labels Oct 22, 2024
@kacheshmarova kacheshmarova requested a review from wnvko October 23, 2024 14:14
@wnvko
Copy link
Contributor

wnvko commented Oct 24, 2024

Setting the collapsed input will not fire opening, opened, closing and closed events. Also banner changes its state without playing any animation. In this way if you:

  • call banner.open a nice animation is played, opening and opened events are fired and banners opens.
  • set collapsed to false no animation is played, no events are fired and banner gets opened.
    Is this behavior desired?

@georgianastasov
Copy link
Contributor Author

Setting the collapsed input will not fire opening, opened, closing and closed events. Also banner changes its state without playing any animation. In this way if you:

  • call banner.open a nice animation is played, opening and opened events are fired and banners opens.
  • set collapsed to false no animation is played, no events are fired and banner gets opened.
    Is this behavior desired?

@wnvko I initially expected this to be the behavior of the collapsed property since it defines the initial state of the banner (expanded or collapsed). I didn’t think that events like opening opened closing or closed should fire when the property is setting the initial state. As for the animations, I’m unsure whether they should be included when toggling collapsed, considering its purpose of defining the starting state.

What’s your suggestion on how this should be handled? Should animations and events be tied to the collapsed property as well?

@wnvko
Copy link
Contributor

wnvko commented Oct 24, 2024

@georgianastasov
The new property could be set at any moment. It is not limited to the initial state of the component. You can even change it during the opening/closing animation of the banner. I am not sure how component should behave in these cases. This should be described in the component specification.

@georgianastasov
Copy link
Contributor Author

@wnvko You're absolutely right, the new collapsed property can be set at any point, even during the banner's open/close animations. I believe the key question here is whether the banner events should be triggered from the collapsed property or not, and if animation should be incorporated during these state transitions. This should definitely be detailed in the component specification to clarify the intended behavior.

@damyanpetev @simeonoff — Would you mind sharing your thoughts on how we should best implement this? Specifically, whether banner events should be triggered from the collapsed property and how to handle animations.

@simeonoff
Copy link
Collaborator

Two things I noticed:

  1. No events should be fired when setting this property programatically. Also, no animations should be triggered initially. Animations trigger only when the property change programmatically on runtime.
  2. To me a boolean property named expanded that is set initially to false sounds more logical.

@damyanpetev
Copy link
Member

I'll echo parts of what @wnvko and @simeonoff already said:

  • I personally agree events shouldn't fire for API calls, though we've had that discussion internally multiple times over the years and never quite settled; it's not entirely consistent across the product even and I don't think we want to open that can of worms for this. But we might not need to, more below.
  • The name collapsed sort of stuck from toggle directive and propagated to multiple components. Can't say I like it much either, more like used to it. There's also the curios case of two other components - the Nav Drawer and Dialog have isOpen which I think is a decent alternative. And don't mind that co-existing with a collapsed getter.
  • Animations honestly a toss depending on the component. For the banner I'm leaning towards without.

Funnily enough, collapsed is already an @Input on the Expansion Panel that does the same thing and it should be possible for the banner to use it and since that doesn't do animations it sets a pattern. The panel itself has a small issue with this being set while the closing animation runs since it sets the value on end, but that's a bit edge case and can be logged separately.

@georgianastasov
Copy link
Contributor Author

georgianastasov commented Nov 20, 2024

After reviewing the discussion, here's the agreed implementation plan:

Property Name

  • Introduce a new @Input property: expanded (default: false).
  • The current collapsed property will remain accessible.

Event Handling

  • No events (opening, opened, closing, closed) will fire when expanded is set programmatically.
  • Events will fire only during user interaction (e.g., clicking to open/close the banner).

Animation Handling

  • Animations will not trigger when expanded is initially set.
  • Animations will trigger when expanded is updated programmatically during runtime.

State Transitions During Animations

  • If expanded is updated during an ongoing animation, the current animation will be canceled, and the new state will be applied immediately without additional animations or events.

@georgianastasov georgianastasov changed the title feat(banner): add collapsed input property to the banner feat(banner): add expanded input property to the banner Nov 20, 2024
Copy link
Contributor

@wnvko wnvko left a comment

Choose a reason for hiding this comment

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

If you change expanded to true while close animation is playing nothing happens!

Here is why. If you have banner which is not collapsed, expanded is true and you set expanded to false this will call close method to the internal expansionPanel. This will then call collapse method and it will start the close animation. At this point collapsed is still false. It will change to true once animation is finish.
While animation is playing if you change expanded to true it will call open method of the internal expansion panel. This will then call expand method. In this method the first check is this:

        if (!this.collapsed) { // If the panel is already opened, do nothing
            return;
        }

As expansion panel's collapsed is still false nothing will happen here.

CHANGELOG.md Outdated Show resolved Hide resolved
@georgianastasov georgianastasov requested a review from wnvko January 14, 2025 12:20
CHANGELOG.md Outdated Show resolved Hide resolved
@wnvko
Copy link
Contributor

wnvko commented Jan 16, 2025

One more thing. Please when creating a PR add descriptive names, like add-expanded-to-banner. Having this at my side does not help me a lot
pr-names

@georgianastasov georgianastasov requested a review from wnvko January 16, 2025 13:41
@Zneeky Zneeky added 💥 status: in-test PRs currently being tested ✅ status: verified Applies to PRs that have passed manual verification and removed ❌ status: awaiting-test PRs awaiting manual verification 💥 status: in-test PRs currently being tested labels Jan 20, 2025
@kacheshmarova kacheshmarova merged commit 69877ca into master Jan 23, 2025
5 checks passed
@kacheshmarova kacheshmarova deleted the ganastasov/feat-14890-master branch January 23, 2025 15:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
banner Banner component version: 19.0.x ✅ status: verified Applies to PRs that have passed manual verification
Projects
None yet
Development

Successfully merging this pull request may close these issues.

IgxBannerComponent - Support collapsed input
7 participants