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

POC - Add tooltip to modal, Modal provider (for computer challenge, language picker, game fork) #2845

Draft
wants to merge 7 commits into
base: devel
Choose a base branch
from

Conversation

andrewjcasal
Copy link

@andrewjcasal andrewjcasal commented Oct 5, 2024

Fixes #

Screen.Recording.2024-10-05.at.3.44.28.PM.mov

Background

  • We had been unable to add tooltips on modals because modals were never a nested component under the main tree containing the DynamicHelp provider.
  • Feel free to try this out by following the video!

Proposed Changes

  • Infrastructure: add the modal provider as a component under the OGSHelpProvider to consumer tooltip information.
  • ActivateTooltip - React component to make it easy to trigger tooltips anywhere. Happy to update if I'm not handling all cases.
  • Modal Context
  • Modal Provider - A context for modals to be rendered at the top of the tree, right under the DynamicHelp functionality. CURRENTLY used only for creating a computer challenge
  • Modal Registry - This will include the list of modals that will use this logic - currently there's only three. Other modals can be added to the registry over time, or can use the register and unregister functions to do this dynamically.
  • ForkModal and LanguagePickerModal switched to functional components to better utilize useContext.
  • Backwards compatibility with other existing modals.

Copy link

github-actions bot commented Oct 5, 2024

Uffizzi Ephemeral Environment Deploying

☁️ https://app.uffizzi.com/github.com/online-go/online-go+com/pull/2845

⚙️ Updating now by workflow run 11506001637.

What is Uffizzi? Learn more!

@anoek
Copy link
Member

anoek commented Oct 12, 2024

I'm switching this to draft until while you work on it, feel free to change it back to ready for merge when you're ready

@anoek anoek marked this pull request as draft October 12, 2024 11:42
@andrewjcasal andrewjcasal marked this pull request as ready for review October 24, 2024 19:17
@andrewjcasal andrewjcasal changed the title POC - Add tooltip to modal, Modal provider (only for computer challenge) POC - Add tooltip to modal, Modal provider (for computer challenge, language picker, game fork) Oct 24, 2024
Copy link
Member

@anoek anoek left a comment

Choose a reason for hiding this comment

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

Aside from the test message, it seems to function well to me. @GreenAsJade does all of this look good from an RDH perspective?

@@ -25,6 +25,9 @@ module.exports = {
"src",
"node_modules"
],
"moduleNameMapper": {
'^@/(.*)': '<rootDir>/src/$1',
Copy link
Member

Choose a reason for hiding this comment

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

Thanks for fixing the tests!


export function ModalHelp(): JSX.Element {
return (
<HelpFlow debug={true} id="modal-help" showInitially={true}>
Copy link
Member

Choose a reason for hiding this comment

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

I think this is the cause of:

image

I think we can probably get rid of that before merging?

Copy link
Contributor

@GreenAsJade GreenAsJade left a comment

Choose a reason for hiding this comment

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

I love the look of Modal implementation now - it cleaned up heaps since last review.

The "test for whether RDH works" is not implemented in the normal RDH way, and it would probably be worth doing it "the RDH way" to totally confirm everything works.

Instead of instantiating a component like ActivateTooltip, you want to get registerTargetItem from the DynamicHelp context and use that to point to the react component that should be annotated. You don't need to "trigger the flow" if you have ShowInitially set, which you do.

This is probably most simply demonstrated in OnlineLeagueSpectatorLanding.tsx and src/views/HelpFlows/OOLSpectatorIntro.tsx .

@GreenAsJade
Copy link
Contributor

@andrewjcasal I realised an even better way of testing this is to get rid of the TBD in GameLog :)

I thought "oh, I will quickly do that right now", but of course that requires GameLogModal to be a new modal, which is not a 2 minute job.

I can do it when I have a moment soonish, or if you add it to the PR, I'll test it out.

GreenAsJade added a commit to GreenAsJade/online-go.com that referenced this pull request Nov 11, 2024
…er (for computer challenge, language picker, game fork)
@GreenAsJade
Copy link
Contributor

See above PR: I made GameLogModal use this, which lets it have RDH about autoscore (🎉). I took out the "test case" RDH, and did a slight refactor as per comment on that commit.

@anoek
Copy link
Member

anoek commented Nov 20, 2024

Marking as draft while stuff is being worked on, mark as ready to merge when everything is set, and thanks again!

@anoek anoek marked this pull request as draft November 20, 2024 12:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants