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

Adding sticker #1172

Merged
merged 14 commits into from
Jan 13, 2024
Merged

Adding sticker #1172

merged 14 commits into from
Jan 13, 2024

Conversation

IndrajeetPatil
Copy link
Collaborator

@IndrajeetPatil IndrajeetPatil commented Dec 22, 2023

Closes #516

  • Finalize background color
  • Finalize text color
  • Build favicons

@codecov-commenter
Copy link

codecov-commenter commented Dec 22, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (e987d7a) 92.16% compared to head (7eaf828) 92.16%.

❗ Current head 7eaf828 differs from pull request most recent head 4ec583d. Consider uploading reports for the commit 4ec583d to get more accurate results

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1172   +/-   ##
=======================================
  Coverage   92.16%   92.16%           
=======================================
  Files          46       46           
  Lines        2655     2655           
=======================================
  Hits         2447     2447           
  Misses        208      208           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@IndrajeetPatil
Copy link
Collaborator Author

@lorenzwalthert Do you know where the changes in inst/WORDLIST are coming from in the precommit workflow?

I don't know how to prevent this.

@olivroy
Copy link
Contributor

olivroy commented Dec 22, 2023

I'd suggest to add favicons.

Locally, you can run pkgdown::build_favicons()

@IndrajeetPatil
Copy link
Collaborator Author

@olivroy Good idea!

Need to wait before the sticker is finalized and then I can do this.

Copy link
Contributor

This is how benchmark results would change (along with a 95% confidence interval in relative change) if e987d7a is merged into main:

  •   :ballot_box_with_check:cache_applying: 168ms -> 169ms [-2.54%, +3.43%]
  •   :ballot_box_with_check:cache_recording: 550ms -> 548ms [-1.79%, +1.07%]
  •   :ballot_box_with_check:without_cache: 1.05s -> 1.05s [-1.18%, +1.19%]

Further explanation regarding interpretation and methodology can be found in the documentation.

@lorenzwalthert
Copy link
Collaborator

Thanks @IndrajeetPatil and sorry for the delay. Great to have this programatically. I changed to following:

  • Adapted to the colours that were in the initial blog post mentioned in A {styler} sticker #516 (comment)
  • Swapped the suit icon, sorry my reference was wrong.
  • excluded png from the spelling hook and re-ran it to not include a gazillion characters from the png.

I am not sure we need another top level directory. Is hextools standard? I'd prefer to put it under an existing directory, like man/figures/ or under inst/ or other but add it to .Rbuildignore.

Copy link
Contributor

github-actions bot commented Jan 7, 2024

This is how benchmark results would change (along with a 95% confidence interval in relative change) if e987d7a is merged into main:

  •   :ballot_box_with_check:cache_applying: 191ms -> 191ms [-0.61%, +1.08%]
  •   :ballot_box_with_check:cache_recording: 574ms -> 575ms [-0.55%, +1.09%]
  •   :ballot_box_with_check:without_cache: 1.07s -> 1.07s [-0.7%, +0.79%]

Further explanation regarding interpretation and methodology can be found in the documentation.

@IndrajeetPatil
Copy link
Collaborator Author

Thanks, @lorenzwalthert! The new colors look good to me.

Can you please also update the script hextools/hexsticker.R for these changes? That way, we can reproduce the sticker in future as well.

I am not sure we need another top level directory. Is hextools standard? I'd prefer to put it under an existing directory, like man/figures/ or under inst/ or other but add it to .Rbuildignore.

Yes, that'd fine by me.

Copy link
Contributor

github-actions bot commented Jan 7, 2024

This is how benchmark results would change (along with a 95% confidence interval in relative change) if e987d7a is merged into main:

  •   :ballot_box_with_check:cache_applying: 150ms -> 151ms [-0.89%, +1.62%]
  •   :ballot_box_with_check:cache_recording: 515ms -> 516ms [-0.3%, +0.71%]
  •   :ballot_box_with_check:without_cache: 988ms -> 986ms [-0.62%, +0.13%]

Further explanation regarding interpretation and methodology can be found in the documentation.

@lorenzwalthert
Copy link
Collaborator

Done @IndrajeetPatil.

@IndrajeetPatil IndrajeetPatil marked this pull request as ready for review January 13, 2024 07:59
Copy link
Contributor

This is how benchmark results would change (along with a 95% confidence interval in relative change) if e987d7a is merged into main:

  •   :ballot_box_with_check:cache_applying: 162ms -> 161ms [-3.54%, +1.58%]
  •   :ballot_box_with_check:cache_recording: 559ms -> 555ms [-2.31%, +1.08%]
  •   :ballot_box_with_check:without_cache: 1.01s -> 1.01s [-0.74%, +0.76%]

Further explanation regarding interpretation and methodology can be found in the documentation.

NAMESPACE Outdated Show resolved Hide resolved
@IndrajeetPatil
Copy link
Collaborator Author

@lorenzwalthert This is ready for a merge from my end. Let me know if you have any other comments.

Copy link
Contributor

This is how benchmark results would change (along with a 95% confidence interval in relative change) if e987d7a is merged into main:

  •   :ballot_box_with_check:cache_applying: 162ms -> 161ms [-2.12%, +1.95%]
  •   :ballot_box_with_check:cache_recording: 538ms -> 538ms [-0.9%, +0.83%]
  •   :ballot_box_with_check:without_cache: 1.01s -> 1.02s [-0.38%, +0.74%]

Further explanation regarding interpretation and methodology can be found in the documentation.

Copy link
Collaborator

@lorenzwalthert lorenzwalthert left a comment

Choose a reason for hiding this comment

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

Thanks so much for keep pushing this forward, it’s the most visible change of {styler} in a while.

@IndrajeetPatil IndrajeetPatil merged commit 17f91fc into main Jan 13, 2024
18 checks passed
@IndrajeetPatil IndrajeetPatil deleted the add-sticker branch January 13, 2024 11:36
@IndrajeetPatil
Copy link
Collaborator Author

The sticker isn't showing up on the devel version of the website. Looking into it.

@lorenzwalthert
Copy link
Collaborator

Maybe try build locally and see if that works?

@lorenzwalthert
Copy link
Collaborator

Also I tried to see the website in incognito mode and I can confirm I can’t see it either.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

A {styler} sticker
4 participants