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

Collapse improvements #127

Merged
merged 1 commit into from
Oct 20, 2023
Merged

Collapse improvements #127

merged 1 commit into from
Oct 20, 2023

Conversation

Gerych1984
Copy link
Contributor

@Gerych1984 Gerych1984 commented Sep 28, 2023

Q A
Is bugfix?
New feature? ✔️
Breaks BC? ✔️
  1. New Collapse widget
  2. New AbstractToggleWidget for widgets with toggler
  3. New AbstractCloseButtonWidget for widgets with optional close button
  4. Accordion and NavBar now using Collapse instead of generate html tags
  5. Replace NavBar::offcanvas(?Offcanvas $offcanvas) to NavBar::withWidget(Offcanvas|Collapse|null $widget)
  6. Collapse and NavBar now extended from AbstractToggleWidget
  7. Modal and Offcanvas now extended from AbstractCloseButtonWidget

@what-the-diff
Copy link

what-the-diff bot commented Sep 28, 2023

PR Summary

  • AbstractToggleWidget Class Added

    • A new file AbstractToggleWidget.php was introduced which contains an abstract class for managing UI toggling behavior. It includes properties such as options, label and methods that enable you to manipulate these properties.
  • Refactoring and Enhancement for Accordion Class

    • The Accordion.php file was modified. The class now extends Widget and the constructor has been modified to declare new properties. Moreover, several methods have been renamed for consistency while some were removed as they were rendered redundant with the recent changes. The overall concept was to streamline the rendering process and make the Accordion feature more effective.
  • New Collapse Class

    • A new file Collapse.php was introduced which is a class extending AbstractToggleWidget. It comprises of methods to prepare options for rendering and a method to render the collapse element.
  • Major Overhaul for Modal class

    • The Modal.php file underwent quite a few changes. Now, it extends the AbstractToggleWidget. It has seen changes in methods and properties that aim to make the class more efficient and easier to work with. The changes also removed quite a few outdated methods.
  • NavBar Class Updated

    • The NavBar.php file was modified to extend AbstractToggleWidget instead of Widget introducing properties for handling the rendering of Offcanvas or Collapse widget. Several methods underwent changes and a few were removed and replaced with a more efficient version.
  • Offcanvas Class Tweaked

    • The Offcanvas.php file was updated to extend AbstractToggleWidget instead of Widget. It introduces properties and methods to manage the toggle button
  • New Test Cases Added

    • A new test file CollapseTest.php was added to ensure the proper functioning of the Collapse class.
    • Other test files like ModalTest.php, NavBarTest.php and OffcanvasTest.php have been updated with additional test scenarios to reflect the changes in their respective classes.

This Pull Request provides significant updates making the code more efficient and organized, simplifying several classes' functioning and integrating improved test scenarios for better development and potential troubleshooting.

@codecov
Copy link

codecov bot commented Sep 28, 2023

Codecov Report

Attention: 3 lines in your changes are missing coverage. Please review.

Comparison is base (e74b93e) 97.61% compared to head (5ebae88) 98.46%.

❗ Current head 5ebae88 differs from pull request most recent head 9ffc0a2. Consider uploading reports for the commit 9ffc0a2 to get more accurate results

Additional details and impacted files
@@             Coverage Diff              @@
##             master     #127      +/-   ##
============================================
+ Coverage     97.61%   98.46%   +0.84%     
- Complexity      472      497      +25     
============================================
  Files            18       21       +3     
  Lines          1637     1692      +55     
============================================
+ Hits           1598     1666      +68     
+ Misses           39       26      -13     
Files Coverage Δ
src/AbstractToggleWidget.php 100.00% <100.00%> (ø)
src/Modal.php 98.15% <100.00%> (+1.44%) ⬆️
src/NavBar.php 100.00% <100.00%> (ø)
src/Offcanvas.php 98.85% <100.00%> (+3.95%) ⬆️
src/AbstractCloseButtonWidget.php 96.66% <96.66%> (ø)
src/Accordion.php 98.38% <97.67%> (+4.63%) ⬆️
src/Collapse.php 98.75% <98.75%> (ø)

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@Gerych1984 Gerych1984 force-pushed the collapse branch 3 times, most recently from 885a60c to 242f817 Compare September 29, 2023 14:35
Copy link
Member

@vjik vjik left a comment

Choose a reason for hiding this comment

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

New abstract classes more like traits. Suggest refactor to traits. @Gerych1984 what do you think?

@Gerych1984
Copy link
Contributor Author

Gerych1984 commented Oct 2, 2023

New abstract classes more like traits. Suggest refactor to traits. @Gerych1984 what do you think?

I've been thinking about it. There are a few nuances

  1. Need an abstract method with the name of the component (bsTarget - traits can set such)
  2. Need to be sure that the trait is used in a class that implements the id() method to set the data-bs-target attribute for the switch (Add another abstract method to it?).
  3. Right now I can't check if it's possible to create a close button outside the widget. But if it is (and that's why I made its button renderer public), then this trait will need id() method too
  4. For bc i used protected properties and replace it in some widgets (Like $toggleLabel = 'Show' by default in Modal). With traits that would be problematic.

src/Modal.php Outdated Show resolved Hide resolved
@samdark samdark merged commit dfbd972 into yiisoft:master Oct 20, 2023
13 of 15 checks passed
@samdark
Copy link
Member

samdark commented Oct 20, 2023

👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants