Skip to content

Redesign and change position of reporter options #360

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

Closed
wants to merge 9 commits into from

Conversation

vedhav
Copy link
Contributor

@vedhav vedhav commented Jul 15, 2025

Copy link
Contributor

github-actions bot commented Jul 15, 2025

badge

Code Coverage Summary

Filename                  Stmts    Miss  Cover    Missing
----------------------  -------  ------  -------  -----------------------------------------------------------------------------------------------------
R/AddCardModule.R           147       2  98.64%   171, 208
R/ContentBlock.R             15       2  86.67%   43-49
R/DownloadModule.R          236      67  71.61%   95-101, 150, 181-186, 195-200, 203-208, 217-222, 225-230, 238-243, 246-251, 258-263, 266-271, 310-314
R/FileBlock.R                13       0  100.00%
R/HTMLBlock.R                 9       0  100.00%
R/LoadReporterModule.R       66      19  71.21%   61-66, 69-74, 80-85, 97
R/NewpageBlock.R              2       0  100.00%
R/PictureBlock.R             30       2  93.33%   20, 118
R/Previewer.R               369      97  73.71%   96-98, 101-102, 179-208, 212-214, 217, 284, 299, 301-304, 307, 310-320, 434-478
R/RcodeBlock.R               18       0  100.00%
R/Renderer.R                121      37  69.42%   97-112, 215, 225, 234, 236-257
R/ReportCard.R               86       3  96.51%   250, 255, 280
R/Reporter.R                107       6  94.39%   270-275
R/ResetModule.R              54       0  100.00%
R/SimpleReporter.R           29       0  100.00%
R/TableBlock.R                9       0  100.00%
R/TextBlock.R                15       0  100.00%
R/utils.R                   126      86  31.75%   7, 38-97, 99, 102-109, 137, 161-169, 206-215
R/yaml_utils.R               81       2  97.53%   78, 289
R/zzz.R                      13      12  7.69%    2-13, 18-20
TOTAL                      1546     335  78.33%

Diff against main

Filename                  Stmts    Miss  Cover
----------------------  -------  ------  --------
R/DownloadModule.R           -3       0  -0.36%
R/LoadReporterModule.R      -38       0  -10.52%
R/SimpleReporter.R           -3       0  +100.00%
TOTAL                       -44       0  -0.60%

Results for commit: 67c5af3

Minimum allowed coverage is 80%

♻️ This comment has been updated with latest results

Copy link
Contributor

github-actions bot commented Jul 15, 2025

Unit Tests Summary

  1 files   19 suites   26s ⏱️
197 tests 196 ✅ 0 💤 0 ❌ 1 🔥
335 runs  334 ✅ 0 💤 0 ❌ 1 🔥

For more details on these errors, see this check.

Results for commit 04e29c0.

♻️ This comment has been updated with latest results.

Copy link
Contributor

github-actions bot commented Jul 15, 2025

Unit Test Performance Difference

Test Suite $Status$ Time on main $±Time$ $±Tests$ $±Skipped$ $±Failures$ $±Errors$
PreviewerReportModule 💚 $1.84$ $-1.09$ $-2$ $0$ $0$ $+1$

Results for commit 3a20221

♻️ This comment has been updated with latest results.

Copy link
Contributor

@gogonzo gogonzo left a comment

Choose a reason for hiding this comment

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

Stuff from report_previewer_ui has been removed but report_previewer_srv has been left untouched. If encoding panel is not there anymore then everything related to encoding panel should be removed. There is a lot of redundant code on the server side now.
Looking at the consequences - report_previewer_srv should probably have only id and reporter arguments which requires a deprecation.

@vedhav
Copy link
Contributor Author

vedhav commented Jul 15, 2025

Thanks for pointing it out. Yes, I think we can remove other params from reporter_previewer_srv. And I see that the reporter_previewer_module is no longer needed in teal as well.

@gogonzo
Copy link
Contributor

gogonzo commented Jul 15, 2025

I think we can remove other params from reporter_previewer_srv

This is a breaking change and a violation of the deprecation process. We shouldn't just remove arguments from public functions. What are alternatives?

And I see that the reporter_previewer_module is no longer needed in teal as well.

It is used as teal.reporter::reporter_previewer_ui/srv are called

@vedhav
Copy link
Contributor Author

vedhav commented Jul 15, 2025

By remove, I mean deprecate and not use them. In teal where they are called, we are not making use of it.

@gogonzo gogonzo closed this Jul 16, 2025
@gogonzo gogonzo deleted the redesign-ui-ux@main branch July 16, 2025 09:47
@github-actions github-actions bot locked and limited conversation to collaborators Jul 16, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants