Modal add #13

n-galrion · 2023-02-10T23:48:35Z

Adds in basic confirm/decline modal.

Example of Usage:

import { Component, Solid, createSignal, Show } from "solid-js";
import { JSX } from "solid-js/jsx-runtime";
import Modal from '../../shared/Modal'
import PageHeader from '../../shared/PageHeader'
const [showModal, setShowModal] = createSignal(false)

const handleConfirm = () => {
  console.log('Modal confirmed')
}

const ModalTest = () => (
  <div>
    <button id="showModal" onClick={() => setShowModal(true)}> Show Modal </button>
    <Show when={showModal()}>
      <Modal messageHeader="Confirm Action" message="Are you sure you want to confirm this action?" confirmText="Confirm" declineText="Cancel" onConfirm={handleConfirm} />
    </Show>
  </div>
)

export default ModalTest;

Adds in a basic Confirm/Deny modal with customizable text.

0x000011b

Awesome, thank you! Before merging though, could you run the code style/quality enforcement tools as described in the README?

# auto-fixes any style problems
$ pnpm run style:fix

# auto-fixes any linting problems, and prints the ones that can't be auto-fixed
$ pnpm run lint:fix

# runs the TypeScript compiler so any type errors will be shown
$ pnpm run typecheck

n-galrion · 2023-02-12T03:20:23Z

The first style fix passed, but the last two failed. Unsure of what I changed that could cause this.

wyrmisis · 2023-02-15T16:01:28Z

src/shared/Modal.tsx

+            </div>
+            <div class="black mb-4 text-lg">{props.message}</div>
+            <div class="flex items-center justify-end">
+              <button


Could there be a case where we may need more or less buttons than two? For example, a modal with information that doesn't need a confirm/deny action.

Yep, I can also foresee situations where e.g. the button(s) should be disabled until a certain condition is met, and things of the sort.

I think the best interface for that will likely imply breaking the Modal component apart into Modal.Header, Modal.Body and Modal.Footer so we have fine-grained control of the contents in each through the children prop, but I think that can be left as a future PR if necessary.

0x000011b

Thanks. Don't worry about the linter complaining about the default export for the mock file, the mocking is being reworked in a separate PR so that point will be moot in a bit. Left some other comments below.

0x000011b · 2023-02-18T21:56:14Z

src/shared/Modal.tsx

+        <div class="fixed inset-0 transition-opacity">
+          <div class="absolute inset-0 bg-gray-500 opacity-75" />
+        </div>


Does this need to have a different z-index? On my machine, the background actually shows in front of the modal and stops me from interacting with it:

0x000011b · 2023-02-18T21:58:40Z

src/shared/Modal.tsx

+            </div>
+            <div class="black mb-4 text-lg">{props.message}</div>
+            <div class="flex items-center justify-end">
+              <button


Yep, I can also foresee situations where e.g. the button(s) should be disabled until a certain condition is met, and things of the sort.

I think the best interface for that will likely imply breaking the Modal component apart into Modal.Header, Modal.Body and Modal.Footer so we have fine-grained control of the contents in each through the children prop, but I think that can be left as a future PR if necessary.

n-galrion added 3 commits February 10, 2023 16:49

Create Modal.tsx

3a7f1bc

Adds in a basic Confirm/Deny modal with customizable text.

Change test variables so Modal functions outside of test page

c09ec27

Clean Up Code

7a61647

0x000011b self-requested a review February 12, 2023 01:29

0x000011b assigned n-galrion Feb 12, 2023

0x000011b added the enhancement New feature or request label Feb 12, 2023

0x000011b reviewed Feb 12, 2023

View reviewed changes

After Run of AutoFixes

3bd11dd

wyrmisis reviewed Feb 15, 2023

View reviewed changes

0x000011b suggested changes Feb 18, 2023

View reviewed changes

0x000011b linked an issue Feb 18, 2023 that may be closed by this pull request

Implement a Modal component #9

Open

n-galrion closed this May 15, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Modal add #13

Modal add #13

n-galrion commented Feb 10, 2023

0x000011b left a comment

n-galrion commented Feb 12, 2023 •

edited

Loading

wyrmisis Feb 15, 2023

0x000011b Feb 18, 2023

0x000011b left a comment

0x000011b Feb 18, 2023

0x000011b Feb 18, 2023

Modal add #13

Modal add #13

Conversation

n-galrion commented Feb 10, 2023

0x000011b left a comment

Choose a reason for hiding this comment

n-galrion commented Feb 12, 2023 • edited Loading

wyrmisis Feb 15, 2023

Choose a reason for hiding this comment

0x000011b Feb 18, 2023

Choose a reason for hiding this comment

0x000011b left a comment

Choose a reason for hiding this comment

0x000011b Feb 18, 2023

Choose a reason for hiding this comment

0x000011b Feb 18, 2023

Choose a reason for hiding this comment

n-galrion commented Feb 12, 2023 •

edited

Loading