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

PomodoroTimer: Fix tests and potentially refactor component #190

Open
2 of 4 tasks
briangershon opened this issue Feb 25, 2023 · 1 comment
Open
2 of 4 tasks

PomodoroTimer: Fix tests and potentially refactor component #190

briangershon opened this issue Feb 25, 2023 · 1 comment
Labels
help wanted Extra attention is needed

Comments

@briangershon
Copy link
Member

briangershon commented Feb 25, 2023

The PomodoroTimer tests were failing.

Discovered during upgrade to NextJS 13 in #189

Investigation

Essentially the tests were testing the internal implementation -- but when this was wrapped with Popover logic, that broke the tests -- buttons were hidden, and as I discovered, you can have React Testing library grab "hidden" buttons, but then aria-label is ignored.

So I came up with what I thought was the perfect plan --- split the component into two -- one wrapped and one not, which fixed the tests that only had to test internal component.

The only problem is that the business logic is split over the Popover (which displays a changing icon) and the internal implementation -- so needs refactoring.

TODOs

  • fix tests
  • refactor to test internal logic and separate out the Popover wrapper
  • refactor to bring back feature to show "status" as an icon in the Popover
  • add GitHub Action to run tests
@briangershon briangershon self-assigned this Feb 25, 2023
@briangershon briangershon changed the title PomodoroTimer: Fix tests and refactor PomodoroTimer: Fix tests and refactor component Feb 28, 2023
@briangershon
Copy link
Member Author

Instead of Refactor

I started to refactor, but I think faster solution would be to figure out how to "hover over the pop-up" in the react tests -- I assume that then unhides the controls the tests need, and they would pass. In this case, the component would not need to be refactored.

I was not able to get the "unhide component by clicking popover" in the tests to work.

Or if refactoring

Here are two ideas:

My refactor in #191 works, but doesn't update the mainIcon -- I was just going to pass a callback down to the component, and when status changed I'd update mainIcon in Popover, but that re-renders component, which causes havoc to the timer, so don't think that's a working approach.

Or refactor might be something more like Context that would hold the reducer and state logic -- and could be tested -- that then simplifies the component tree which could get all it needs from Context.

@briangershon briangershon changed the title PomodoroTimer: Fix tests and refactor component PomodoroTimer: Fix tests and potentially refactor component Feb 28, 2023
@briangershon briangershon added the help wanted Extra attention is needed label Feb 28, 2023
@briangershon briangershon removed their assignment Feb 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Extra attention is needed
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant