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.

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

Range filters should be improved to work mostly client side #2827

Merged
merged 6 commits into from
Dec 19, 2024
Merged

Range filters should be improved to work mostly client side #2827

merged 6 commits into from
Dec 19, 2024

Conversation

mgbaybay
Copy link
Contributor

Changes

  • Range filters should be improved to work mostly client side
  • Reports changes

Relates to: xibosignage/xibo#2732

@mgbaybay mgbaybay self-assigned this Dec 10, 2024
@mgbaybay mgbaybay marked this pull request as ready for review December 10, 2024 08:37
@mgbaybay mgbaybay force-pushed the feature/kopff_range_filter_improvements branch from a96896f to 7fcbd6f Compare December 10, 2024 08:48
dasgarner
dasgarner requested changes Dec 10, 2024
View reviewed changes
Copy link
Member

@dasgarner dasgarner left a comment

Choose a reason for hiding this comment

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

This is going in the right direction, although I have a few suggestions/comments to look at please.

views/forms.twig Outdated Show resolved Hide resolved
lib/Report/ApiRequests.php Outdated Show resolved Hide resolved
lib/Report/ApiRequests.php Show resolved Hide resolved
views/forms.twig Outdated Show resolved Hide resolved
reports/apirequests-report-form.twig Outdated Show resolved Hide resolved
reports/apirequests-report-form.twig Show resolved Hide resolved
@mgbaybay mgbaybay force-pushed the feature/kopff_range_filter_improvements branch 2 times, most recently from 4761324 to 39b4a39 Compare December 11, 2024 09:54
@mgbaybay mgbaybay force-pushed the feature/kopff_range_filter_improvements branch from 39b4a39 to 8361006 Compare December 11, 2024 09:57
@mgbaybay mgbaybay requested a review from dasgarner December 11, 2024 10:00
dasgarner
dasgarner requested changes Dec 11, 2024
View reviewed changes
Copy link
Member

@dasgarner dasgarner left a comment

Choose a reason for hiding this comment

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

I have one more comment here, and I think @maurofmferrao needs to review the JS please.

views/forms.twig Outdated
<i class="fa fa-calendar"></i>
</div>
<input class="form-control dateControl date rangeInput"
type="text" name="fromDt" id="fromDt"
Copy link
Member

Choose a reason for hiding this comment

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

Great, this reusable control is much better! Do we need to think about making the IDs here be specific to this control? If we include this macro more than once we will currently have duplicated IDs. The name would need to stay the same to it matches the API, but the ID could be unique to this instance (perhaps we could make the ID fromDt_name?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi, Dan! Thank you for this feedback! Please see updated changes here :)

@mgbaybay mgbaybay force-pushed the feature/kopff_range_filter_improvements branch from c8fea00 to ec9e3da Compare December 11, 2024 13:45
@mgbaybay mgbaybay requested a review from dasgarner December 11, 2024 13:46
dasgarner
dasgarner reviewed Dec 12, 2024
View reviewed changes
Copy link
Member

@dasgarner dasgarner left a comment

Choose a reason for hiding this comment

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

This looks good to me, I'll leave it for Mauro to do the final review

maurofmferrao
maurofmferrao requested changes Dec 12, 2024
View reviewed changes
Copy link
Member

@maurofmferrao maurofmferrao left a comment

Choose a reason for hiding this comment

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

Just some minor changes regarding the code formatting, everything else looks great! 👍🏻

ui/src/core/xibo-cms.js Outdated Show resolved Hide resolved
ui/src/core/xibo-cms.js Outdated Show resolved Hide resolved
ui/src/core/xibo-cms.js Outdated Show resolved Hide resolved
ui/src/core/xibo-cms.js Outdated Show resolved Hide resolved
maurofmferrao
maurofmferrao requested changes Dec 12, 2024
View reviewed changes
Copy link
Member

@maurofmferrao maurofmferrao left a comment

Choose a reason for hiding this comment

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

Some more code formatting changes. 🙏🏻

ui/src/core/xibo-cms.js Outdated Show resolved Hide resolved
ui/src/core/xibo-cms.js Outdated Show resolved Hide resolved
maurofmferrao
maurofmferrao previously approved these changes Dec 12, 2024
View reviewed changes
Copy link
Member

@maurofmferrao maurofmferrao left a comment

Choose a reason for hiding this comment

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

Looks good to me! ✅

@dasgarner
Copy link
Member

@maurofmferrao how do you recommend this PR is tested? should the test wait until we've retargeted it to develop?

@maurofmferrao
Copy link
Member

@maurofmferrao how do you recommend this PR is tested? should the test wait until we've retargeted it to develop?

It might work for the new develop, and we can test now for the current kopff just to make sure it works. But maybe to avoid retesting we do that ( wait for 4.2 to become develop and then test+merge )

@ifarzana
Copy link
Contributor

TEST: /Reporting END

@github-actions GitHub Actions
Copy link

Test Suite: failed ❌
https://github.com/xibosignage/xibo-cms/actions/runs/12367439853

@ifarzana
Copy link
Contributor

TEST: /Reporting/report_timeconnectedsummary.cy.js END

@github-actions GitHub Actions
Copy link

Test Suite: failed ❌
https://github.com/xibosignage/xibo-cms/actions/runs/12371948368

@ifarzana
Copy link
Contributor

The cypress test of Time Connected Summary report needs to be updated to reflect the new changes. I am signing off on this work for now, and we will address the failed test later.

@ifarzana
Copy link
Contributor

TEST: login.cy.js END

@github-actions GitHub Actions
Copy link

Test Suite: succeeded ✅
https://github.com/xibosignage/xibo-cms/actions/runs/12375716972

@mgbaybay mgbaybay changed the base branch from kopff to develop December 19, 2024 09:26
@mgbaybay mgbaybay dismissed maurofmferrao’s stale review December 19, 2024 09:26

The base branch was changed.

maurofmferrao
maurofmferrao approved these changes Dec 19, 2024
View reviewed changes
Copy link
Member

@maurofmferrao maurofmferrao left a comment

Choose a reason for hiding this comment

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

Looks good to me!

@dasgarner dasgarner merged commit 1bec7bb into develop Dec 19, 2024
4 of 5 checks passed
@dasgarner dasgarner deleted the feature/kopff_range_filter_improvements branch December 19, 2024 14:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@maurofmferrao maurofmferrao maurofmferrao approved these changes

@dasgarner dasgarner Awaiting requested review from dasgarner

Assignees

@mgbaybay mgbaybay

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@mgbaybay @dasgarner @maurofmferrao @ifarzana