-
Notifications
You must be signed in to change notification settings - Fork 6
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
Uninstrument Text and RichText #934
Comments
I reviewed
I also reviewed instances of
Also, autoselect is working well on the navbar. The sim title routes users to I'm not sure who should be on the hook for these changes. Maybe @zepumph? Maybe @pixelzoom since Acid-Base Solutions is likely next up in the queue? |
If this ends up on my worklist, I won't be able to get to it until >= August 28. It's a little too cross-cutting for me to tackle while I'm on reduced hours. |
Today @kathy-phet asked if I could take the lead, and I agreed. @zepumph can review. This will be part of the CaV and greenhouse SHAs. |
Today we agreed that the navigation bar title text should be hide-able. |
Fixed in the commits, @arouinfar can you please review/test? @zepumph would you like to code review? If we need migration rules (deletes), we can write them lazily. |
Thanks @samreid! We reviewed with @kathy-phet @samreid @zepumph in Studio and the changes look good, closing. |
In slack, @kathy-phet said:
|
Alright. We can confirm that migration rules are now complete and working for this issue. Closing |
Reopening. These TODOs appear in beers-law-lab-migration-rules.ts and concentration-migration-rules.ts. This was discovered while working on phetsims/beers-law-lab#332, and complicated that issue. // Delete items that no longer exist.
// TODO: https://github.com/phetsims/joist/issues/934 Some of these should be renames or other rules // Uninstrument Text and RichText. These were copied from beers-law-lab-migration-rules.ts
// Delete items that no longer exist.
// TODO: https://github.com/phetsims/joist/issues/934 Some of these should be renames or other rules. Note there are also unused rules since
// this was copied from beers-law-lab |
Also wondering why this was not reopened automatically. Is the automated task still running that looks for TODOs? |
Great question. It was working in phetsims/greenhouse-effect#361 (comment). I will take a look. |
I was going to work on this next, but the comment in https://github.com/phetsims/phet-io/issues/1963#issuecomment-1714788272 indicates that maybe @pixelzoom is working on it. So I'll self unassign and check in at standup tomorrow. |
I do not plan to do any further work on this. Or anything else related to migration until this is addressed. |
Ahh, because the feature relies on the lint rule called |
Why is "todo-should-have-issue" turned off in phet-io-sim-specific? Shall we turn it on? |
@samreid will take the lead on the TODO that is left. |
It was addressed in https://github.com/phetsims/phet-io-sim-specific/commit/43e849416cec874431b41893f7bdb88f741ef0ff, closing. |
In https://github.com/phetsims/phet-io/issues/1952#issuecomment-1664495275 we decided that we generally don't want to instrument Text nodes, and instead just want to focus on the stringProperties, especially if they are derived or pattern.
Most likely the important things are for general sim/screen names, and for HomeScreen.
The text was updated successfully, but these errors were encountered: