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

Adds Technical Blueprint to pkgdown site #923

Merged
merged 28 commits into from
Oct 12, 2023
Merged

Conversation

averissimo
Copy link
Contributor

@averissimo averissimo commented Sep 21, 2023

Pull Request

Fixes https://github.com/insightsengineering/coredev-tasks/issues/467

Preview

https://averissimo.github.io/teal/index.html

(deployed to personal repo)

Problems to solve

  • Show mermaid diagrams
  • Show dot diagram
  • Resize images to flow better in HTML
    • Alternative: use custom css

Changes description

  • Adds pages just like rlang

Screenshot

image

@averissimo
Copy link
Contributor Author

averissimo commented Sep 26, 2023

Preview here: https://averissimo.github.io/teal/

Note that links to the source vignettes are dead (as they don't yet existing on main branch)

@averissimo averissimo marked this pull request as ready for review September 26, 2023 14:04
@github-actions
Copy link
Contributor

github-actions bot commented Sep 26, 2023

badge

Code Coverage Summary

Filename                         Stmts    Miss  Cover    Missing
-----------------------------  -------  ------  -------  ----------------------------------------------------------------------------------
R/dummy_functions.R                 88      63  28.41%   9-76, 101-104, 107-111
R/get_rcode_utils.R                 46       1  97.83%   49
R/include_css_js.R                  24       0  100.00%
R/init.R                            77      28  63.64%   171-178, 183-204, 216-218
R/module_filter_manager.R          107      29  72.90%   62-70, 79-84, 228, 233-246
R/module_nested_tabs.R             170      16  90.59%   72, 119, 123-124, 138-145, 163, 216, 238, 271
R/module_snapshot_manager.R        148     105  29.05%   71-82, 109-118, 122-134, 136-143, 149-164, 177-200, 203-214, 217-223, 237, 258-281
R/module_tabs_with_filters.R        67       1  98.51%   95
R/module_teal_with_splash.R         33       2  93.94%   65, 77
R/module_teal.R                    164      12  92.68%   68, 71, 158-159, 209-210, 230-233, 235, 239
R/modules_debugging.R               18      18  0.00%    25-44
R/modules.R                        128      22  82.81%   206-211, 222-226, 341-384
R/reporter_previewer_module.R       17       2  88.24%   23, 27
R/show_rcode_modal.R                20      20  0.00%    16-37
R/tdata.R                           41       2  95.12%   146, 172
R/teal_reporter.R                   60       5  91.67%   65, 116-117, 120, 137
R/teal_slices.R                     57      12  78.95%   118-131
R/utils.R                           21       0  100.00%
R/validate_inputs.R                 32       0  100.00%
R/validations.R                     60      37  38.33%   111-373
R/zzz.R                             11       7  36.36%   3-14
TOTAL                             1389     382  72.50%

Diff against main

Filename      Stmts    Miss  Cover
----------  -------  ------  --------
TOTAL             0       0  +100.00%

Results for commit: 99ffacd

Minimum allowed coverage is 80%

♻️ This comment has been updated with latest results

@averissimo averissimo requested a review from a team September 28, 2023 13:53
Copy link
Contributor

@donyunardi donyunardi left a comment

Choose a reason for hiding this comment

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

couple small comments. Thanks for working on this!

Copy link
Contributor

Choose a reason for hiding this comment

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

Is this a duplicate of blueprint/reporter.jpg?

vignettes/blueprint/ddl.Rmd Outdated Show resolved Hide resolved
vignettes/blueprint/qenv.Rmd Show resolved Hide resolved
Copy link
Contributor

@chlebowa chlebowa left a comment

Choose a reason for hiding this comment

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

Nicely done and ripe for finishing touches 👍

However, I find name thoroughly misleading. A blueprint is a design to be followed when building something, not an explanation of the inner workings of a mechanism.

vignettes/blueprint/index.Rmd Outdated Show resolved Hide resolved
vignettes/blueprint/actors.Rmd Outdated Show resolved Hide resolved
vignettes/blueprint/dataflow.Rmd Outdated Show resolved Hide resolved
vignettes/blueprint/dataflow.Rmd Outdated Show resolved Hide resolved
vignettes/blueprint/products_map.Rmd Outdated Show resolved Hide resolved
vignettes/blueprint/filter_panel.Rmd Outdated Show resolved Hide resolved
vignettes/blueprint/ddl.Rmd Show resolved Hide resolved
vignettes/blueprint/module_encapsulation.Rmd Outdated Show resolved Hide resolved
vignettes/blueprint/module_encapsulation.Rmd Outdated Show resolved Hide resolved
vignettes/blueprint/module_encapsulation.Rmd Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
@pawelru
Copy link
Contributor

pawelru commented Oct 3, 2023

There was a discussion in the issue about how to do that and I just wanted to point out that I really like the way you proposed here. It enables (some) quarto features that we need with minimal workarounds (e.g. import mermaid lib without e.g. modifying docs workflow). Nicely done!
Now we need to polish the content and then it's good to go from my perspective

donyunardi and others added 11 commits October 5, 2023 13:41
Co-authored-by: Pawel Rucki <[email protected]>
Signed-off-by: Dony Unardi <[email protected]>
Co-authored-by: Aleksander Chlebowski <[email protected]>
Signed-off-by: Dony Unardi <[email protected]>
Co-authored-by: Aleksander Chlebowski <[email protected]>
Signed-off-by: Dony Unardi <[email protected]>
Co-authored-by: Aleksander Chlebowski <[email protected]>
Signed-off-by: Dony Unardi <[email protected]>
Co-authored-by: Aleksander Chlebowski <[email protected]>
Signed-off-by: Dony Unardi <[email protected]>
Co-authored-by: Aleksander Chlebowski <[email protected]>
Signed-off-by: Dony Unardi <[email protected]>
Co-authored-by: Aleksander Chlebowski <[email protected]>
Signed-off-by: Dony Unardi <[email protected]>
Co-authored-by: Aleksander Chlebowski <[email protected]>
Signed-off-by: Dony Unardi <[email protected]>
Co-authored-by: Aleksander Chlebowski <[email protected]>
Signed-off-by: Dony Unardi <[email protected]>
Co-authored-by: Aleksander Chlebowski <[email protected]>
Signed-off-by: Dony Unardi <[email protected]>
Co-authored-by: Aleksander Chlebowski <[email protected]>
Signed-off-by: Dony Unardi <[email protected]>
@github-actions
Copy link
Contributor

github-actions bot commented Oct 5, 2023

Unit Tests Summary

    1 files    16 suites   13s ⏱️
171 tests 171 ✔️ 0 💤 0
341 runs  341 ✔️ 0 💤 0

Results for commit 99ffacd.

♻️ This comment has been updated with latest results.

README.md Outdated Show resolved Hide resolved
@pawelru
Copy link
Contributor

pawelru commented Oct 6, 2023

FYI: r-lib/pkgdown#2210 (comment)

Co-authored-by: Pawel Rucki <[email protected]>
Signed-off-by: Dony Unardi <[email protected]>
@vedhav
Copy link
Contributor

vedhav commented Oct 9, 2023

I enjoyed going through the Technical blueprint vignettes. I have a few comments:

  1. If we're calling it the "Technical" blueprint I think it should also contain a vignette to help a aid new developer contribute by talking about our pre-commit hooks and a little about the coding standards we follow, along with how to effectively set up their local (This was not very straight forward while getting started as we have to navigate through different repos often, maybe we can provide helper scripts and hacks that we use)
  2. Reading the qenv vignette did not give me a complete idea of how it actually works in creating reproducible code. I think it's worth mentioning that any executed code is appended and stored which is later executed to produce the output in the teal app, The diagram could have been a little clearer by adding the code based on their sequence of use (1. R library calls, 2. Data preprocessing, ...).
  3. I would have also liked a mermaid diagram that goes a little deeper to explain the crux of "Creating a module" as an extension to the qenv in a practical way.

Please note that these are just suggestions that are not intended to block this PR, I'm happy to create a new discussion issue for this.

Copy link
Contributor

@donyunardi donyunardi left a comment

Choose a reason for hiding this comment

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

@vedhav
Thanks for the review.

This is meant for teal Technical Blueprint, not so much about the technical aspect on collaboration (we have a different md for that). We could change the title to teal Technical Blueprint on the menu bar, but I wonder if it would seem repeated.

These articles are meant to cover teal concepts at a high-level. Any deeper knowledge of a topic should be directed to the respective R packages. I changed some wording around qenv and added a link to creating custom module vignettes to see qenv usage. If we need a diagram about this, maybe we can add it here?

Finally, can I ask your assistance to review these suggested changes?

vignettes/blueprint/index.Rmd Outdated Show resolved Hide resolved
vignettes/blueprint/module_encapsulation.Rmd Outdated Show resolved Hide resolved
vignettes/blueprint/qenv.Rmd Outdated Show resolved Hide resolved
vignettes/blueprint/qenv.Rmd Outdated Show resolved Hide resolved
vignettes/blueprint/qenv.Rmd Show resolved Hide resolved
vedhav and others added 6 commits October 10, 2023 10:38
Co-authored-by: Dony Unardi <[email protected]>
Signed-off-by: Vedha Viyash <[email protected]>
Co-authored-by: Dony Unardi <[email protected]>
Signed-off-by: Vedha Viyash <[email protected]>
Co-authored-by: Dony Unardi <[email protected]>
Signed-off-by: Vedha Viyash <[email protected]>
Co-authored-by: Dony Unardi <[email protected]>
Signed-off-by: Vedha Viyash <[email protected]>
@donyunardi donyunardi enabled auto-merge (squash) October 11, 2023 00:47
@vedhav vedhav dismissed chlebowa’s stale review October 12, 2023 03:11

addressed them

@donyunardi donyunardi merged commit d2177e5 into main Oct 12, 2023
22 checks passed
@donyunardi donyunardi deleted the 467_add_blueprint@main branch October 12, 2023 03:12
@chlebowa chlebowa mentioned this pull request Oct 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants