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

Modal for links #389

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Modal for links #389

wants to merge 3 commits into from

Conversation

SharonStrats
Copy link
Contributor

This is a modal.
if you have an array of links [{ label: 'string', link: 'string'}, ... ]
You call createWindow(Dom, listofLinks) and it will return the modal. To display the modal by clicking a button add modal.display = 'block' to the onClickFunction. This has not been tested yet, due to some other technical issues.

I have also created a modalStyle file next to the modal. I think this is how we should do it as we go forward so that the styles are next to the file we are styling when possible.

TODO: change the styling so that when the user hovers on li the li has a background color.

Copy link
Contributor

@jeff-zucker jeff-zucker left a comment

Choose a reason for hiding this comment

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

[EDIT : ignore this, it was my setup]

Haven't finished testing yet, but these changes allow it to compile without errors

Line 8 of modals.ts

  • change ../jss to ../lib/jss

Line 1 modalsStyles.ts

  • change ** to /*

@angelo-v
Copy link
Contributor

angelo-v commented Apr 2, 2021

Cool. Could you please add unit tests and also stories to storybook?

@timbl
Copy link
Contributor

timbl commented Apr 17, 2023

Is this a modal in the sense that it block all interaction for the user until the choice has been made? If so then I would prefer that it is not. It should allow the user to work on other things while the choice has not been made yet. Maybe it already does.

See https://www.w3.org/DesignIssues/UserInterface.html#modals
Also good to pass the function a DOM element near or under ot on top of which to make the pop-up so the choice doesn't get lost in the wrong part of the UI.

Copy link
Contributor

@TallTed TallTed left a comment

Choose a reason for hiding this comment

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

clarity changes to comments, and adding some // comment markers to lines without them in larger current /* ... */ comment blocks (so if the /* ... */ wrappers around the code are removed, these comment lines don't break the build)

Comment on lines +23 to +24
Window click handler so that the modal will close
even if the user doesn't click close
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Window click handler so that the modal will close
even if the user doesn't click close
// Window click handler so that the modal will close
// even if the user doesn't click close.

Comment on lines +15 to +16
// Two functions that need to be implemented to use the modal
// When the user clicks the button, open the modal
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// Two functions that need to be implemented to use the modal
// When the user clicks the button, open the modal
// Two functions that need to be implemented to use the modal.
// When the user clicks the button, open the modal.

modal.style.display = "block";
}

// When the user clicks anywhere outside of the modal, close it
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// When the user clicks anywhere outside of the modal, close it
// When the user clicks anywhere outside of the modal, close it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: In Progress
Development

Successfully merging this pull request may close these issues.

6 participants