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

Update ComponentUtilities.php, updated method accept "null" value for second parameter #1663

Closed
wants to merge 14 commits into from

Conversation

viliusvsx
Copy link
Contributor

@viliusvsx viliusvsx commented Feb 21, 2024

Sometimes I update variables with null values. Without this PR, I get an error, that I cannot pass the null value in the "updated" method

P.S. I don't think that test is failing because of my PR.

All Submissions:

  • Have you followed the guidelines in our Contributing document?
  • Have you checked to ensure there aren't other open Pull Requests for the same update/change?

New Feature Submissions:

  1. Does your submission pass tests and did you add any new tests needed for your feature?
  2. Did you update all templates (if applicable)?
  3. Did you add the relevant documentation (if applicable)?
  4. Did you test locally to make sure your feature works as intended?

Changes to Core Features:

  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you written new tests for your core changes, as applicable?
  • Have you successfully ran tests with your changes locally?

lrljoe and others added 14 commits November 3, 2023 16:02
Add Initial Lifecycle Hooks - Configuring/Configured by @lrljoe in v3 - Initial Lifecycle Hooks - Configuring/Configured #1520
Add "HasAllTraits" for Maintainability by @lrljoe in v3 - Splitting Codebase - ConfigurableAreas, CollapsingColumns and TableAttributes #1514
Rename row-contents blades for clarity by @lrljoe in V3 Rename row-contents blades #1519
Add missing tests for DateFilter and DateTimeFilter by @lrljoe in V3 Adds missing tests for DateTimeFilter and DateFilter #1527
- Add additional Lifecycle Hook by @lrljoe in #1534
  - SettingColumns/ColumnsSet
- Migrate methods for pre-render out of render by @lrljoe in #1534
- Update tests to reflect hooks by @lrljoe in #1534
- Update tests to add invalid string tests for dates by @lrljoe in #1534
- Remove maps and minimise functions from FrontendAssets by @lrljoe in #1534
Develop to Master - Workflows Only
V3 - Develop to Master (3.1.3)
## [v3.1.4] - 2023-12-04
### New Features
- Add capability to hide Column Label by @lrljoe in #1512
- Add capability to set a custom script path for the scripts/styles by @lrljoe in #1557
- Added rowsRetrieved Lifecycle Hook, expanded documentation for Lifecycle Hooks

### Bug Fixes
- Added missing tailwind background colour class for when hovering over the clear button in dark mode by @slakbal in #1553
- Fixed extraneous space in config.php by @viliusvsx in in #1577
- Changed table default vertical overflow to auto by @dmyers in #1573
- Fix footer rendering issue with extra td displayed depending on bulk action statuses

### Tweaks
- Create additional Exception Classes (NoColumnsException, NoSearchableColumnsException, NoSortableColumnsException)
- Revert previous splitting of JS Files
- Add capability to customise Bulk Actions Styling with tests by @lrljoe in #1564
  - TH Classes
  - TH Checkbox Classes
  - TD Classes
  - TD Checkbox Classes
@viliusvsx viliusvsx changed the title Update ComponentUtilities.php Update ComponentUtilities.php, updated method accept "null" value for second parameter Feb 22, 2024
@lrljoe
Copy link
Collaborator

lrljoe commented Feb 24, 2024

Good point. Let me check the Livewire Core code, but shouldn't be an issue to merge this bit in.

I will be separating this bit out so that it's a bit more maintainable, but that's on my to-do list rather than anything else

Will need to test it, but may make it into the next release, worth noting that you should check if it's "null", before checking it it's equal to "", just to avoid any Exceptions being thrown!

NOTE: Changing base to "develop", as that's where things get merged in

@lrljoe lrljoe changed the base branch from master to develop February 24, 2024 01:18
@lrljoe
Copy link
Collaborator

lrljoe commented Feb 24, 2024

Having swiftly reviewed the PR, I think there's a better way, which is to advance a pending update:

So the present chunk of code relating to "search" in the updated() method in ComponentUtilities is replaced with this in "WithSearch":

    public function updatedSearch(string|array|null $value): void
    {
        $this->resetComputedPage();

        // Clear bulk actions on search
        $this->clearSelected();
        $this->setSelectAllDisabled();

        if (is_null($value) || $value === '') {
            $this->clearSearch();
        }
    }

This way, we segregate off the "updated()" logic into the relevant Trait.

Unless you have any objection, I'll use the above approach to get the fix in. Please do reach out on the official Discord if you want to discuss it further!

@lrljoe
Copy link
Collaborator

lrljoe commented Feb 24, 2024

Have created the following PR that uses my above logic:
#1666

@lrljoe lrljoe added the duplicate This issue or pull request already exists label Feb 24, 2024
@lrljoe lrljoe closed this Feb 24, 2024
@viliusvsx viliusvsx deleted the patch-2 branch February 24, 2024 10:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
duplicate This issue or pull request already exists
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants