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

Shared trash can images #320

Open
jonathanolson opened this issue Jul 13, 2017 · 24 comments
Open

Shared trash can images #320

jonathanolson opened this issue Jul 13, 2017 · 24 comments

Comments

@jonathanolson
Copy link
Contributor

It looks like there are three trash can images, and they are all shared:

balancing-act:
trash-can

forces-and-motion-basics:
trash-can

energy-skate-park-basics:
trash-can
trash-can-disabled

pendulum-lab:
trash-can
trash-can-disabled

@jbphet and @samreid, presumably these should all be moved to scenery-phet. Preferences on names, or objections?

@samreid
Copy link
Member

samreid commented Jul 14, 2017

Scenery-phet sounds good. Not sure if we should break apart the trash icon and arrow, or use font awesome for the trash can part.

@jbphet
Copy link
Contributor

jbphet commented Jul 14, 2017

The one for Balancing Act has an asset file too in Adobe Illustrator format. This should probably be moved as well. You'll probably need to update the licensing files as well. No strong opinions on the names - trash-can or something similar would be fine.

@jbphet jbphet removed their assignment Jul 17, 2017
@samreid
Copy link
Member

samreid commented Aug 1, 2017

Maybe we should separate out the curved arrow from the trash icon and create it as a kite shape? Perhaps some sim in the future will want that arrow to be a different color. I'd also be interested in seeing the black trash can as a kite shape so it is scaleable and paintable.

Here's the font awesome trash icon, can/should we use that?
http://fontawesome.io/icon/trash/
image

@jonathanolson let me know if you'd like other feedback or if you want me to take the lead on this.

@jonathanolson
Copy link
Contributor Author

@ariel-phet, does that trash can icon in the above comment (#320 (comment)) look like a good enough replacement for the trash can in the icons for pendulum-lab/energy-skate-park-basics?

I'd like to either replace to a version using the font-awesome icon, or consolidate the images so they aren't copied across sims.

@ariel-phet
Copy link

@jonathanolson that icon does look like a good enough to replacement to me

@jonathanolson
Copy link
Contributor Author

Moved over ClearThermalButton to scenery-phet, as it was shared and benefited from refactoring (takes care of the bottom pair of images).

@pixelzoom
Copy link
Contributor

@jonathanolson What's the status of this? Looks like you did some work on this about a year ago.

@jonathanolson
Copy link
Contributor Author

I handled the ESPB/pendulum-lab refactor. I didn't yet handle the balancing-act/FAMB refactor (I wasn't working on either of those sims, and at the time needed to handle it quickly for pendulum-lab).

@pixelzoom
Copy link
Contributor

FYI... CCK, Equality Explorer and MOTHA are all using new FontAwesomeNode( 'trash',...). For example in CCK:

screenshot_954

@pixelzoom
Copy link
Contributor

pixelzoom commented Dec 30, 2018

@jonathanolson said:

Moved over ClearThermalButton to scenery-phet ...

Recommended to chose a different name. While it may be used for thermal energy in your sim(s), the implementation in scenery-phet/ClearThermalButton contains nothing specific to thermal energy. It's simply a button with a trash can + arrow on it, and the client handles the specifics of when to enable/disable it.

@pixelzoom
Copy link
Contributor

There's also a TODO in ClearThermalButton with no associated issue number:

    var arrowShape = new CurvedArrowShape( 10, -0.9 * Math.PI, -0.2 * Math.PI, {
      tandem: options.tandem.createTandem( 'arrowShape' ),
      headWidth: 12,
      tailWidth: 4
      // TODO
    } );

jonathanolson added a commit that referenced this issue Jan 3, 2019
@jonathanolson
Copy link
Contributor Author

Sorry about the random TODO, no clue what that is for, so I removed it (things seem to be working fine).

This button includes the "return" arrow node (its main feature), which the CCK/EqEx/MOTHA cases I don't believe would be used. It also has default colors meant for its specific purpose.

So presumably it would need to be generalized to support "trash can-like buttons", for which I don't see any significant advantage.

@pixelzoom
Copy link
Contributor

pixelzoom commented Jan 3, 2019

I don't understand. How is a button that looks like this:

trash-can

... in any way specific to thermal energy, or any specific quantity for that matter? Why couldn't it be used to clear anything that is color-coded to the arrow color?

If it's supposed to be specific to thermal energy, then why is baseColor an option?

Additionally, the doc for ClearThermalButton is:

A button meant for conditionally being able to clear thermal energy from a system. Has a trash can with an arrow, and appears disabled if there is no thermal energy.

This doc (and the type name) make it sound like ClearThermalButton is observing thermal energy, and is responsible for the enabled/disabled state. It has no such responsibility, it's not observing anything, and the client is fully responsible for changing the state.

@pixelzoom
Copy link
Contributor

I renamed MoveToTrashButton and its usages in energy-skate-park, masses-and-springs, and pendulum-lab. In general I did not rename the associated variable name. For example in ENERGY_SKATE_PARK/PieChartLegend:

    var clearThermalButton = new MoveToTrashButton( {
      tandem: tandem.createTandem( 'clearThermalButton' ),
      ...
    } ); 

The purpose of the button in this context is to clear thermal energy, so the variable name clearThermalButton is appropriate, as is the matching tandem name.

@jonathanolson please review.

@pixelzoom pixelzoom assigned jonathanolson and unassigned pixelzoom Jan 8, 2019
pixelzoom added a commit that referenced this issue Jan 8, 2019
Signed-off-by: Chris Malley <cmalley@pixelzoom.com>
@pixelzoom
Copy link
Contributor

Ah... Over in #455, I discovered that this button doesn't have any way to change the arrow color. It's hard-coded to thermal energy (using the incorrect color btw). So I'll add an option for the arrow fill.

pixelzoom added a commit that referenced this issue Jan 8, 2019
Signed-off-by: Chris Malley <cmalley@pixelzoom.com>
@pixelzoom
Copy link
Contributor

pixelzoom commented Jan 8, 2019

options.arrowColor has been added, it defaults to the correct color for thermal energy (PhetColorScheme.HEAT_THERMAL_ENERGY), and I've added a demo to ButtonsScreenView.

Over to @jonathanolson.

pixelzoom added a commit that referenced this issue Jan 9, 2019
Signed-off-by: Chris Malley <cmalley@pixelzoom.com>
@pixelzoom
Copy link
Contributor

pixelzoom commented Jan 9, 2019

It turns out the none of the sims using MoveToTrashButton were using PhetColorScheme.HEAT_THERMAL_ENERGY - see #456. So I changed those sims to set arrowColor explicitly for their MoveToTrashButton instances, and I changed the default to arrowColor: 'black'.

pixelzoom added a commit that referenced this issue Jan 9, 2019
Signed-off-by: Chris Malley <cmalley@pixelzoom.com>
@pixelzoom
Copy link
Contributor

Status of this issue: Assigned to @jonathanolson to review since 1/2019. Raising prioirty of that review to high.

@jonathanolson
Copy link
Contributor Author

These changes look good, and works well for the simulations that use it!

@jbphet and @jessegreenberg, thoughts on whether we should have a shared trash can image for FAMB/balancing-act (that those sims effectively share, but potentially at different sizes)?

@jbphet
Copy link
Contributor

jbphet commented Mar 1, 2021

My thought would be that it would probably be better to have a shared image if we can come up with a scale that works for both sims. If not, it doesn't seem like a big deal. This would be worth maybe 1/2 hr of work.

@jbphet jbphet removed their assignment Mar 1, 2021
@pixelzoom
Copy link
Contributor

Reviewing scenery-phet issues for Q4 2022 planning...

Looks like this stalled out in Feb 2021, and is waiting on feeback from @jessegreenberg. @kathy-phet please prioritize.

@jessegreenberg
Copy link
Contributor

It looks like FAMB and balancing-act are the same images with different scales already. I am fine moving this to common code but it doesn't seem like a big improvement. That trash can looks dated and I doubt we will use that image again in a sim.

Its probably a quick task though, @kathy-phet would you like someone to do this?

@jessegreenberg jessegreenberg removed their assignment Oct 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

No branches or pull requests

7 participants