Skip to content
This repository was archived by the owner on May 8, 2025. It is now read-only.

Conversation

@hunter-pbs
Copy link

Instead of toggling pause state on application focus, just set pause to true when focus is lost, requiring a manual unpause via pause buttons.

@hunter-pbs hunter-pbs requested a review from a team as a code owner January 3, 2025 19:24
@hunter-pbs hunter-pbs requested review from fuentism, pbs-jmo, renzoolguin and sjosephpbs and removed request for a team January 3, 2025 19:24
@fuentism
Copy link

fuentism commented Jan 6, 2025

@hunter-pbs this behavior makes sense to me, but I didn't see this functionality listed on that ticket - is this something that Kelly is aware of, that we're changing what happens when you re-focus the play page?

@hunter-pbs
Copy link
Author

@hunter-pbs this behavior makes sense to me, but I didn't see this functionality listed on that ticket - is this something that Kelly is aware of, that we're changing what happens when you re-focus the play page?

Sorry, I referenced the wrong Pause ticket in the changelog. Has been updated. This behavior is detailed here:
https://www.pivotaltracker.com/story/show/188461849

fuentism
fuentism previously approved these changes Jan 6, 2025
Copy link

@fuentism fuentism left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, but let's let @renzoolguin weigh in on this one too since this will be our first commit to the container code. Woo!

renzoolguin
renzoolguin previously approved these changes Jan 6, 2025
Copy link
Member

@renzoolguin renzoolguin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good. I was a little confused at first because it looked we were PR'ing into the original repo and not the fork due to the similar repo names. Maybe we could at least update the README to reflect that this is for the redesign site?

@hunter-pbs hunter-pbs dismissed stale reviews from renzoolguin and fuentism via 1cd61fb January 6, 2025 17:03
@hunter-pbs
Copy link
Author

hunter-pbs commented Jan 6, 2025

This looks good. I was a little confused at first because it looked we were PR'ing into the original repo and not the fork due to the similar repo names. Maybe we could at least update the README to reflect that this is for the redesign site?

@renzoolguin @fuentism
I added a Redesign tag to that Changelog entry. Is that sufficient? Should we add a line noting when we forked and took over? Should our version numbers more clearly represent our fork? Like, is this Website Redesign Springroll Container v1.0.0? I can imagine conflicting version numbers getting confusing down the road.

@renzoolguin
Copy link
Member

This looks good. I was a little confused at first because it looked we were PR'ing into the original repo and not the fork due to the similar repo names. Maybe we could at least update the README to reflect that this is for the redesign site?

@renzoolguin @fuentism I added a Redesign tag to that Changelog entry. Is that sufficient? Should we add a line noting when we forked and took over? Should our version numbers more clearly represent our fork? Like, is this Website Redesign Springroll Container v1.0.0? I can imagine conflicting version numbers getting confusing down the road.

Let's start tracking the Redesign work as 3.0.0+ Note in that version that is the start of redesign work. That way any updates in 2.x.y can be merged in if any changes in the original repo need to be pulled in.

@hunter-pbs hunter-pbs merged commit 005ee21 into main Jan 6, 2025
@fuentism fuentism deleted the feature/pause-on-blur branch January 9, 2025 16:35
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants