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

make it possible to call Luxor when outside rendering #443

Merged
merged 10 commits into from
May 26, 2022

Conversation

Wikunia
Copy link
Member

@Wikunia Wikunia commented Nov 7, 2021

PR Checklist

If you are contributing to Javis.jl, please make sure you are able to check off each item on this list:

  • Did I update CHANGELOG.md with whatever changes/features I added with this PR?
  • Did I make sure to only change the part of the file where I introduced a new change/feature?
  • Did I cover all corner cases to be close to 100% test coverage (if applicable)?
  • Did I properly add Javis dependencies to the Project.toml + set an upper bound of the dependency (if applicable)?
  • Did I properly add test dependencies to the test directory (if applicable)?
  • Did I check relevant tutorials that may be affected by changes in this PR?
  • Did I clearly articulate why this PR was made the way it was and how it was made?

Link to relevant issue(s)
Closes #438

How did you address these issues with this PR? What methods did you use?
I've added a CURRENTLY_RENDERING const same as @gpucce in #434 and check in each function in luxor_overrides.jl whether we are inside a rendering. If not the standard Luxor function is called.

More checking would be good. Maybe we find some old issues where this issue was raised.

@Wikunia
Copy link
Member Author

Wikunia commented Nov 7, 2021

Seems like there is some issue with MacOS and text maybe.

@codecov-commenter
Copy link

codecov-commenter commented Nov 7, 2021

Codecov Report

Merging #443 (48b0e41) into main (9ecec81) will increase coverage by 0.01%.
The diff coverage is 100.00%.

❗ Current head 48b0e41 differs from pull request most recent head 7f0e25b. Consider uploading reports for the commit 7f0e25b to get more accurate results

@@            Coverage Diff             @@
##             main     #443      +/-   ##
==========================================
+ Coverage   96.35%   96.37%   +0.01%     
==========================================
  Files          35       35              
  Lines        1620     1627       +7     
==========================================
+ Hits         1561     1568       +7     
  Misses         59       59              
Impacted Files Coverage Δ
src/Javis.jl 96.92% <100.00%> (+0.02%) ⬆️
src/luxor_overrides.jl 96.34% <100.00%> (+0.23%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9ecec81...7f0e25b. Read the comment docs.

Copy link
Member

@gpucce gpucce left a comment

Choose a reason for hiding this comment

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

Other than that small thing I think it works

src/Javis.jl Outdated Show resolved Hide resolved
@TheCedarPrince
Copy link
Member

Hey @Wikunia and @gpucce !
I'd love to add my thoughts but I really am not quite sure as to what this PR is doing?
Does it just allow one to create Luxor drawings alongside Javis?
Could you maybe give me a small example here @Wikunia ?

Thanks!

~ tcp 🌳

@Wikunia
Copy link
Member Author

Wikunia commented Jan 18, 2022

Hey @TheCedarPrince
currently on the move so for now only this example in our test cases:

@testset "Normal Luxor Drawing" begin

The idea is to be able to play around with Luxor before going into animations in the same file. With this PR one is able to do Luxor drawings by importing Javis but then just use Luxor functions without any of Javis objects and stuff.

Hope that makes sense

@Wikunia Wikunia changed the base branch from master to main February 25, 2022 18:35
@Wikunia
Copy link
Member Author

Wikunia commented Feb 25, 2022

Would love to have this in v1.0 of Javis 😉
What do you think @TheCedarPrince ?

@Sov-trotter Sov-trotter self-requested a review April 24, 2022 15:35
@Wikunia Wikunia merged commit d05bc1e into main May 26, 2022
@Wikunia Wikunia deleted the feature-luxor-without-rendering branch May 26, 2022 09:53
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.

Making Luxor work completely inside Javis
6 participants