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

[skip vbump] upversion to v0.6.0 #248

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Conversation

donyunardi
Copy link
Contributor

Fixes #234

@donyunardi donyunardi added the core label Feb 4, 2025
Copy link
Contributor

github-actions bot commented Feb 4, 2025

Unit Tests Summary

  1 files   24 suites   6s ⏱️
193 tests 193 ✅ 0 💤 0 ❌
679 runs  679 ✅ 0 💤 0 ❌

Results for commit 55f2651.

Copy link
Contributor

github-actions bot commented Feb 4, 2025

Unit Tests Summary

  1 files   24 suites   8s ⏱️
194 tests 194 ✅ 0 💤 0 ❌
697 runs  697 ✅ 0 💤 0 ❌

Results for commit f4c273f.

♻️ This comment has been updated with latest results.

@donyunardi donyunardi requested a review from a team February 4, 2025 01:02
Copy link

@llrs-roche llrs-roche left a comment

Choose a reason for hiding this comment

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

Locally it ran without any problem or NOTE. I mostly checked the vignettes.

  • The image on the "Getting started" vignette is cut, and the table columns are hard to read:

image

It is an svg file, so it can be edited to fix this.

  • I also found a couple of screenshots that do not match current output. I think we are adding shinylive instead of screenshots of vignettes whenever possible. Not sure if we want to do that before release too.

Comparing v0.6.0 vs 0.5.0, I see some other changes that might be worth mentioning on NEWS:

  • Some log functions went from trace to debug, while some where from log_warn to warning (resolve.delayed_value_choices()).
  • Some data files were removed and it reuses data from other packages (teal.data)

Signed-off-by: Dony Unardi <donyunardi@gmail.com>
@donyunardi
Copy link
Contributor Author

donyunardi commented Feb 5, 2025

The image on the "Getting started" vignette is cut, and the table columns are hard to read:

I believe you're referring to the "Combining Data Extract with Data Merge" vignette, not "Getting Started" vignette:
https://insightsengineering.github.io/teal.transform/main/articles/data-extract-merge.html

I don't see what you're seeing:
image

I did shrink my browser width and it was still responsive nicely.

I also found a couple of screenshots that do not match current output.

The output changes because we replaced the example data.

I think we are adding shinylive instead of screenshots of vignettes whenever possible. Not sure if we want to do that before release too.

Yes, let's do it! I'll take care of this.

I see some other changes that might be worth mentioning on NEWS:

Thanks, I'll add it to NEWS.

@llrs-roche
Copy link

Yes, I confused the vignettes name and I was referring to the "Combining Data Extract with Data Merge".
I'm using the work computer with Chrome version 132.0.6834.160 (Official Build) (64-bit) on Windows. Maybe to avoid this issue instead of a svg we could use the png (but keep the svg on the repository for easy editing?). The png I think would look the same for all devices.

donyunardi added a commit that referenced this pull request Feb 6, 2025
Addressing this comment:

#248 (review)

I'm creating a separate PR to merge the vignettes into the main branch
first so they can be built before moving forward with the release.

### Summary
* Update all vignettes to include shinylive 
* Update NEWS 
* Add roxy.shinylive to Suggest

For this comment:
> while some where from log_warn to warning
(resolve.delayed_value_choices()).

I decided not to add to NEWS because the affected function is internal.
Signed-off-by: Dony Unardi <donyunardi@gmail.com>
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.

[CRAN Release]: 0.6.0
2 participants