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

More light bulb designs and implementations? #679

Open
pixelzoom opened this issue Mar 8, 2021 · 5 comments
Open

More light bulb designs and implementations? #679

pixelzoom opened this issue Mar 8, 2021 · 5 comments

Comments

@pixelzoom
Copy link
Contributor

Discovered while working on phetsims/scenery-phet#170. That issue has a lot of discussion about the benefits of consolidating lightbulb styles, and the desire to use common code for CCK. So I was dismayed to find that CCK not only punted on the problem, but has made the problem much worse by creating (at least) 3 new classes and 4 new PNG files related to lightbulbs:

  • CustomLightBulbNode.js
  • CCKCLightBulbNode.js
  • LightBulbSocketNode.js
  • lightbulb-middle.png
  • lightbuld-middle-high.png
  • lightbuld-middle-icon.png
  • lightbuld-middle-real.png

CustomLightBulbNode.js is documented as "Forked from SCENERY_PHET/LightBulbNode", a nice way of saying "Copied". That's not going to fly for code review.

I can understand if this needed to be done to meet a milestone. But I don't see any TODO comments or Git issue indicating that this is a temporary situation, or that there's a plan to go back and align this with goals of phetsims/scenery-phet#170. So I'm creating this issue.

@samreid What is the plan for light bulb in CCK?

@pixelzoom pixelzoom changed the title More light bulb More light bulb designs and implementations? Mar 8, 2021
@samreid
Copy link
Member

samreid commented Jul 9, 2021

As mentioned in phetsims/scenery-phet#170 (comment)

I'm tracking the CCK aspect of this issue in #679 and will wait for this issue to be planned as a quarterly goal before going further. I confirmed this issue is listed in the spreadsheet. Self-unassigning.

@samreid samreid removed their assignment Jul 9, 2021
@samreid
Copy link
Member

samreid commented Jul 29, 2021

Labeling for the AC milestone for the purposes of discussion today.

@samreid samreid added this to the AC 1.0 milestone Jul 29, 2021
@samreid samreid self-assigned this Aug 20, 2021
@samreid
Copy link
Member

samreid commented Aug 25, 2021

Question for design meeting: Is it OK to leave the light bulbs as they are until we complete the quarterly goal of organizing the common code light bulbs in #679 ?

@samreid samreid removed their assignment Aug 25, 2021
@arouinfar
Copy link
Contributor

If there is a quarterly goal related to phetsims/scenery-phet#170, we should have a meeting between designers/relevant developers to inventory the current implementations and discuss the learning goals/design needs for these variations. I personally don't think we should delay CCK: AC for a common code issue that has been sitting around for 6 years. If the scenery issue is indeed blocking, we need to devote the resources to unblock it and move CCK: AC along.

@samreid
Copy link
Member

samreid commented Aug 26, 2021

At today's design meeting, we agreed not to delay CCK for the common code light bulbs. Removing from the milestone.

@samreid samreid removed this from the AC 1.0 milestone Aug 26, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants