Skip to content

Simplification of kiosk chart test #23115

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
Open
wants to merge 14 commits into
base: master
Choose a base branch
from
Open

Simplification of kiosk chart test #23115

wants to merge 14 commits into from
+112 −157

Conversation

KuxaBeast
Copy link
Contributor

@KuxaBeast KuxaBeast commented Aug 26, 2025 •
edited
Loading

@KuxaBeast KuxaBeast marked this pull request as draft August 26, 2025 15:33
@github-actions GitHub Actions
Copy link

Great PR! Please pay attention to the following items before merging:

Files matching lib/**.pm:

  • Consider adding or extending unit tests in t/

This is an automatically generated QA checklist based on modified files.

okurz
okurz requested changes Aug 27, 2025
View reviewed changes
@KuxaBeast KuxaBeast force-pushed the kiosk branch 10 times, most recently from cbe8b1d to f4caaed Compare September 4, 2025 03:43
@KuxaBeast KuxaBeast marked this pull request as ready for review September 4, 2025 04:21
pdostal
pdostal approved these changes Sep 4, 2025
View reviewed changes
Copy link
Member

@pdostal pdostal left a comment

Choose a reason for hiding this comment

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

Awesome 🎉

grisu48
grisu48 approved these changes Sep 4, 2025
View reviewed changes
Copy link
Contributor

@grisu48 grisu48 left a comment

Choose a reason for hiding this comment

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

Good job! There are some open topics, but they can also be addressed in another PR, as the intention here is to simplify and that is done beautifully.

ricardobranco777
ricardobranco777 reviewed Sep 4, 2025
View reviewed changes
ricardobranco777
ricardobranco777 approved these changes Sep 4, 2025
View reviewed changes
Copy link
Contributor

@ricardobranco777 ricardobranco777 left a comment

Choose a reason for hiding this comment

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

Minor suggestion. Otherwise LGTM.

GraceWang571
GraceWang571 approved these changes Sep 5, 2025
View reviewed changes
Copy link
Contributor

@GraceWang571 GraceWang571 left a comment

Choose a reason for hiding this comment

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

Nice PR. LGTM.

mloviska
mloviska reviewed Sep 5, 2025
View reviewed changes
mloviska
mloviska approved these changes Sep 5, 2025
View reviewed changes
Copy link
Contributor

@mloviska mloviska left a comment

Choose a reason for hiding this comment

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

just a neatpick, otherwise good to roll

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@grisu48 grisu48 grisu48 approved these changes

@ricardobranco777 ricardobranco777 ricardobranco777 approved these changes

@okurz okurz okurz requested changes

@pdostal pdostal pdostal approved these changes

@GraceWang571 GraceWang571 GraceWang571 approved these changes

@mloviska mloviska mloviska approved these changes

@linguini-dev linguini-dev linguini-dev approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@KuxaBeast @pdostal @grisu48 @okurz @ricardobranco777 @GraceWang571 @mloviska @linguini-dev