Closed Nancy-Salpepi closed 1 year ago
zepumph
commented
1 year ago @ariel-phet recommends an investigation to see if this is pretty straight forward (like an hour or two). Most likely the undo button would go right next to the decay button (maybe on the right). You still would only undo the most recent decay (so pressing the button n times quickly doesn't stack up for a larger undo).
zepumph
commented
1 year ago Looking into this now for a bit.
zepumph
commented
1 year ago Here is a messy patch proof-of-concept. I definitely think this is possible, but we are going to want to work out some of the quirks here.
zepumph
commented
1 year ago Fixed a disposal bug when undoing alpha decay (endedEmitter=>finishEmitter)
zepumph
commented
1 year ago I see @Luisav1 committed above, and there is one TODO. @Luisav1, can you please take a look at this patch and see if it removes the TODO in a good enough way?
Subject: [PATCH] https://github.com/phetsims/build-a-nucleus/issues/189
---
Index: js/common/view/BANScreenView.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/common/view/BANScreenView.ts b/js/common/view/BANScreenView.ts
--- a/js/common/view/BANScreenView.ts (revision b7546c115774d65bf6680a55bc914bf370b3b4e2)
+++ b/js/common/view/BANScreenView.ts (date 1695310287833)
@@ -177,16 +177,14 @@
// hideUndoButtonEmitter in a beta decay
// TODO: hacky, what's a better way to 'defer' the mass number property here only? https://github.com/phetsims/build-a-nucleus/issues/189
let oldMassNumber: number;
- let olderMassNumber: number;
// Hide the undo decay button if anything in the nucleus changes.
- Multilink.multilink( [ this.model.particleAtom.massNumberProperty, this.model.userControlledProtons.lengthProperty,
+ Multilink.lazyMultilink( [ this.model.particleAtom.massNumberProperty, this.model.userControlledProtons.lengthProperty,
this.model.incomingProtons.lengthProperty, this.model.incomingNeutrons.lengthProperty,
this.model.userControlledNeutrons.lengthProperty ], massNumber => {
- if ( !( olderMassNumber === massNumber ) ) {
+ if ( oldMassNumber !== massNumber ) {
BANScreenView.hideUndoButtonEmitter.emit();
}
- olderMassNumber = oldMassNumber;
oldMassNumber = massNumber;
} );This issue is that we alter the atom mass counts async after animating the change, but only for the chart intro. Even if we didn't have the emitter I'm unsure how we would make this work well.
https://github.com/phetsims/build-a-nucleus/blob/b7546c115774d65bf6680a55bc914bf370b3b4e2/js/common/view/BANScreenView.ts#L657-L663 @Luisav1 perhaps we should pair on this.
zepumph
commented
1 year ago Ahhh, @Luisav1 helped me understand the problem better, it is about the chart intro screen's fade in/out, not the nucleon type change.
@Luisav1 here is a patch that cleans up some of the workarounds in your commit, and adds a TODO where we should try out deferring.
zepumph
commented
1 year ago Taking a look now.
zepumph
commented
1 year ago Ok. @Luisav1, the weirdness is in that we were depending on the a model step to add the new particle to the atoms. Is there a way that we could not animate it and then not rely on the animation ending emitter?
Right now the hack I exchanged your hack for is stepping the model, but there must be a way to just add the particle immediately to the particle atom from the stack. Isn't that how we support the initial count query parameters?
Luisav1
commented
1 year ago @zepumph You're right! In the commit above we're now using addNucleonImmediatelyToAtom() to add the new particle and I use fadeAnimation still to fade it in.
For the removal of the old particle though it's harder since we want to fade it out before removal so removeNucleonImmediatelyFromAtom() wouldn't work as it is. So I first fade the particle out then use animateAndRemoveParticle() to remove the particle at the end of the fade animation.
zepumph
commented
1 year ago But the removal doesn't matter too much since it is sync removed from the particleAtom, (which is where the mass is changed). Right?
Luisav1
commented
1 year ago Oh right, yes, that's correct. The old particle is removed from the particleAtom with extractParticle(), followed almost immediately by addNucleonImmediatelyToAtom. Which is why even though the mass number changes, it is rapid enough that nothing in the particleAtom changes afterward to cause the undo button to hide again.
Luisav1
commented
1 year ago Based on a slack conversation of the buttons placement: Ariel Paul 6:54 PM I think the undo button is working well. I cannot say I love the placement. Things just feel a bit unbalanced. I wonder if it would work to center the decay button above the partial chart. I think the undo button would still fit within the panel. Or alternately move the undo button to be more in line with the decay equation? So center the Decay button above the partial chart (instead of the left alignment it has now, and then move the undo button above the decay button, more associated with the decay equation).
Here are some very rough patches (since they both have issues with stringTest=dynamic) for the undo button placement.
Patch and picture of the undo button beside the decay button:
Patch and picture of undo button on top of decay button:
I think with string lengths varying for 'Decay', it would be nicer to have the undo button be above the Decay button.
Luisav1
commented
1 year ago Meeting with @zepumph, there are three things we should know and try for the button placement.
Subject: [PATCH] undo button and decay button up with the decay equation
---
Index: js/chart-intro/view/NuclideChartAccordionBox.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/chart-intro/view/NuclideChartAccordionBox.ts b/js/chart-intro/view/NuclideChartAccordionBox.ts
--- a/js/chart-intro/view/NuclideChartAccordionBox.ts (revision cc8933b1601bd97b681f55a22367255042dc2824)
+++ b/js/chart-intro/view/NuclideChartAccordionBox.ts (date 1695676587100)
@@ -11,7 +11,7 @@
import AccordionBox, { AccordionBoxOptions } from '../../../../sun/js/AccordionBox.js';
import NuclideChartAndNumberLines from './NuclideChartAndNumberLines.js';
import BuildANucleusStrings from '../../BuildANucleusStrings.js';
-import { HBox, Text, VBox } from '../../../../scenery/js/imports.js';
+import { HBox, Rectangle, Text, VBox } from '../../../../scenery/js/imports.js';
import NuclideChartLegendNode from './NuclideChartLegendNode.js';
import { SelectedChartType } from '../model/ChartIntroModel.js';
import BANConstants from '../../common/BANConstants.js';
@@ -31,6 +31,7 @@
import BANScreenView from '../../common/view/BANScreenView.js';
type NuclideChartAccordionBoxOptions = AccordionBoxOptions;
+const X_SPACING_BETWEEN_CHARTS = 10;
class NuclideChartAccordionBox extends AccordionBox {
@@ -119,22 +120,29 @@
} );
BANScreenView.hideUndoButtonEmitter.addListener( () => { undoDecayButton.visible = false; } );
- // Position the focused chart and the decay button together.
- const focusedChartAndButtonVBox = new VBox( {
- children: [
- new HBox( { children: [ decayButton, undoDecayButton ], spacing: 5, excludeInvisibleChildrenFromBounds: false } ),
- focusedNuclideChartNode
- ],
+ const undoDecayAndDecayButtonVBox = new VBox( {
+ children: [ undoDecayButton, decayButton ],
+ spacing: 5,
+ align: 'center',
+ excludeInvisibleChildrenFromBounds: false
+ } );
+
+ // Add the undo decay button and decayEquationNode together in an HBox.
+ const decayEquationAndButtonsHBox = new HBox( {
+ children: [ decayEquationNode, undoDecayAndDecayButtonVBox ],
spacing: 10,
align: 'center'
} );
+ // TODO: why doesn't this work by using the decayEquationAndButtonsHBox directly?
+ const hBoxBounds = new Rectangle( decayEquationAndButtonsHBox.bounds );
+ hBoxBounds.addChild( decayEquationAndButtonsHBox );
// Update the view in the accordion box depending on the selectedNuclideChartProperty.
selectedNuclideChartProperty.link( selectedNuclideChart => {
zoomInNuclideChartNode.visible = selectedNuclideChart === 'zoom';
- focusedChartAndButtonVBox.visible = selectedNuclideChart === 'zoom';
- decayEquationNode.visible = selectedNuclideChart === 'zoom';
+ focusedNuclideChartNode.visible = selectedNuclideChart === 'zoom';
+ hBoxBounds.visible = selectedNuclideChart === 'zoom';
nuclideChartAndNumberLines.visible = selectedNuclideChart === 'partial';
} );
@@ -146,15 +154,16 @@
// done for the "Stable" text in the DecayEquationNode.
zoomInNuclideChartNode,
nuclideChartAndNumberLines,
- focusedChartAndButtonVBox
+ focusedNuclideChartNode
],
- spacing: 10,
- align: 'top',
+ spacing: X_SPACING_BETWEEN_CHARTS,
+ align: 'center',
excludeInvisibleChildrenFromBounds: true
} );
+ decayEquationAndButtonsHBox.preferredWidth = chartsHBox.bounds.width;
const contentVBox = new VBox( {
children: [
- decayEquationNode,
+ hBoxBounds,
chartsHBox,
nuclideChartLegendNode
],
Subject: [PATCH] undo button to left of decay button
---
Index: js/chart-intro/view/NuclideChartAccordionBox.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/chart-intro/view/NuclideChartAccordionBox.ts b/js/chart-intro/view/NuclideChartAccordionBox.ts
--- a/js/chart-intro/view/NuclideChartAccordionBox.ts (revision cc8933b1601bd97b681f55a22367255042dc2824)
+++ b/js/chart-intro/view/NuclideChartAccordionBox.ts (date 1695673652575)
@@ -11,7 +11,7 @@
import AccordionBox, { AccordionBoxOptions } from '../../../../sun/js/AccordionBox.js';
import NuclideChartAndNumberLines from './NuclideChartAndNumberLines.js';
import BuildANucleusStrings from '../../BuildANucleusStrings.js';
-import { HBox, Text, VBox } from '../../../../scenery/js/imports.js';
+import { HBox, Rectangle, Text, VBox } from '../../../../scenery/js/imports.js';
import NuclideChartLegendNode from './NuclideChartLegendNode.js';
import { SelectedChartType } from '../model/ChartIntroModel.js';
import BANConstants from '../../common/BANConstants.js';
@@ -74,6 +74,8 @@
} );
const focusedNuclideChartNode = new FocusedNuclideChartNode(
protonCountProperty, neutronCountProperty, focusedChartTransform, showMagicNumbersProperty );
+ const focusedChartRectangle = new Rectangle( focusedNuclideChartNode.bounds.dilateX( 11 ) );
+ focusedChartRectangle.addChild( focusedNuclideChartNode );
const zoomInNuclideChartNode = new ZoomInNuclideChartNode(
protonCountProperty, neutronCountProperty, zoomInChartTransform, showMagicNumbersProperty );
@@ -97,7 +99,7 @@
fontSize: 14
},
minWidth: 80,
- maxWidth: 150,
+ maxWidth: 120,
listener: () => {
// Do the given decay on the atom, if there is a decay.
@@ -122,12 +124,20 @@
// Position the focused chart and the decay button together.
const focusedChartAndButtonVBox = new VBox( {
children: [
- new HBox( { children: [ decayButton, undoDecayButton ], spacing: 5, excludeInvisibleChildrenFromBounds: false } ),
- focusedNuclideChartNode
+ new HBox( { children: [ undoDecayButton, decayButton ], spacing: 5, excludeInvisibleChildrenFromBounds: true } ),
+ focusedChartRectangle
],
spacing: 10,
align: 'center'
} );
+ undoDecayButton.visibleProperty.link( visible => {
+ if ( visible ) {
+ focusedChartAndButtonVBox.setAlign( 'left' );
+ }
+ else {
+ focusedChartAndButtonVBox.setAlign( 'center' );
+ }
+ } );
// Update the view in the accordion box depending on the selectedNuclideChartProperty.
selectedNuclideChartProperty.link( selectedNuclideChart => {I think My favorite is 3' where the decay and undo buttons are still centered:
Subject: [PATCH] update TODO, https://github.com/phetsims/phet-io-wrappers/issues/507
---
Index: js/chart-intro/view/NuclideChartAccordionBox.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/chart-intro/view/NuclideChartAccordionBox.ts b/js/chart-intro/view/NuclideChartAccordionBox.ts
--- a/js/chart-intro/view/NuclideChartAccordionBox.ts (revision cc8933b1601bd97b681f55a22367255042dc2824)
+++ b/js/chart-intro/view/NuclideChartAccordionBox.ts (date 1695682409317)
@@ -122,7 +122,7 @@
// Position the focused chart and the decay button together.
const focusedChartAndButtonVBox = new VBox( {
children: [
- new HBox( { children: [ decayButton, undoDecayButton ], spacing: 5, excludeInvisibleChildrenFromBounds: false } ),
+ new HBox( { children: [ undoDecayButton, decayButton ], spacing: 5, excludeInvisibleChildrenFromBounds: false } ),
focusedNuclideChartNode
],
spacing: 10,Let's touch base with @ariel-phet.
Luisav1
commented
1 year ago Meeting with @ariel-phet, we will be going with the last option where they are just swapped from their current version. But also want to bring in some internationalization maxWidth's.
Luisav1
commented
1 year ago Move undo button to left of decay button and decrease decay button maxWidth in commit above. Closing.
Test device MacBook Air M1 chip
Operating System 13.5.1
Browser Safari 16.6
Problem description For https://github.com/phetsims/qa/issues/977, I think it would be useful to have an undo button on the Chart Intro screen like there is on the Decay screen. Sometimes, after pressing the Decay button, my eyes are focused on the Nuclide Chart panel and I miss what is happening to the particles.
It looks like there is room next to the Decay button for it (but not sure how dynamic locale would affect that).