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

fix: in cgAdd: make cgLayer be optional #182

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

nytamin
Copy link
Member

@nytamin nytamin commented May 1, 2023

  • What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
    quality of life improvement

  • What is the current behavior? (You can also link to an open issue here)

cgAdd requires the parameters cgLayer and playOnLoad

  • What is the new behavior (if this is a feature change)?

cgLayer is optional and defaults to 1

playOnLoad is optional and defaults to true

  • Other information:

In my experience, cgLayer and playOnLoad are rarely used nowadays, so I think that we should allow for leaving them out and fill in the most-commonly used values.

@nytamin nytamin requested a review from mint-dewit May 1, 2023 04:31
@codecov-commenter
Copy link

codecov-commenter commented May 2, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (278eb2b) 82.03% compared to head (3066ee3) 82.16%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #182      +/-   ##
==========================================
+ Coverage   82.03%   82.16%   +0.13%     
==========================================
  Files          14       14              
  Lines         729      729              
  Branches      140      166      +26     
==========================================
+ Hits          598      599       +1     
+ Misses        130      129       -1     
  Partials        1        1              

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

@mint-dewit
Copy link
Contributor

In my experience, cgLayer and playOnLoad are rarely used nowadays

I don't know if we as library maintainers are allowed to make such assumptions. Loading a template before playing it certainly does not seem like such a crazy idea.

I'm definitely not against it, it would be useful for most of what I do too but it feels like a dangerous decision to make.

src/__tests__/serializers.spec.ts Outdated Show resolved Hide resolved
@Julusian
Copy link
Member

Julusian commented Feb 1, 2024

cgLayer is definitely falling out of use, as it has no effect when using html.
playOnLoad though is still relevant, but as maintainers of the library we can make decisions on what we think are sane defaults. users are still able to overrule those decisions by supplying a value.

I think I do agree that maybe it would be better for playOnLoad to default to false. This method is called cgAdd and there is a separate cgPlay method, so I think it will be more commonly expected for cgAdd to not autoplay

@nytamin nytamin changed the title fix: in cgAdd: make cgLayer and playOnLoad be optional fix: in cgAdd: make cgLayer be optional Feb 22, 2024
@nytamin
Copy link
Member Author

nytamin commented Feb 22, 2024

Good points!
I've revised this PR to only modify the cgLayer to be optional.
After reading your comments, I changed my mind regarding the playOnLoad property, lets keep it mandatory to reduce risk of unexpected behaviour for new users of the library.

@nytamin nytamin requested review from Julusian and mint-dewit and removed request for mint-dewit February 22, 2024 07:00
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.

None yet

4 participants