Review Follow Ups and Refactoring #2

Winggo · 2020-11-17T01:30:39Z

Description of the change

@anvilco/react-signature-frame and @anvilco/react-signature-modal v1.4.3 are published. Finished up docs.
Incorporated review changes
- AnvilSignatureFrame
  - importing css no longer required, default styles are embedded into the component
  - styles can overridden using classNames and ids
  - custom id and className supported through props
  - removed listening for message from staging
  - removed ANVIL_URLS.includes()
  - custom props passed into component supported
  - classNames and ids use kebab-case
- AnvilSignatureFrame
  - @anvilco/react-signature-frame component used within @anvilco/react-signature-modal component
  - modal app element is configurable
Refactored components into class components

Type of change

Bug fix (non-breaking change that fixes an issue)
New feature (non-breaking change that adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)

Related issues

Dev Checklist

The code changed/added as part of this pull request has been covered with tests
All tests related to the changed code pass in development
No previous tests unrelated to the changed code fail in development

- @anvilco/[email protected] - @anvilco/[email protected]

packages/react-signature-frame/src/index.js

packages/react-signature-modal/src/index.js

benogle · 2020-11-19T19:16:23Z

packages/react-signature-modal/src/index.js

 import PropTypes from 'prop-types'
 import ReactModal from 'react-modal'
+import AnvilSignatureFrame from '../../react-signature-frame/src/index.js'
 import DeleteIcon from './components/DeleteIcon'
 import './index.css'


I still dont love that we are always importing styles here. But maybe it's fine for now. The output code will inject a style tag in the dom. Not ideal if they want to replace the styling, but probably fine for now.

There is a way to lazy load components, but looks like react >= 16.6: https://medium.com/@prawira/react-conditional-import-conditional-css-import-110cc58e0da6. We could do this later on and select on a prop like loadDefaultStyles or something

Thought about this for a bit and came up with a better idea that doesn't involve importing css, since we don't want to inject a style tag. I've put the default styles inline using the style prop and added an enableDefaultStyles prop that defaults to true. If users set enableDefaultStyles to false, then all the inline styles are removed and can be overriden.

Like this

<AnvilSignatureFrame style={enableDefaultStyles ? { width: '80vw', height: '85vh', maxWidth: '1200px', borderStyle: 'none', } : undefined} {...anvilFrameProps} signURL={signURL} onLoad={onLoad} onFinish={onFinish} anvilURL={anvilURL} />

Ok noted. I think there is a more flexible way to do the width and height in the modal then. Maybe use width/height 100% then the users can size the modal and the iframe will react accordingly

It works the opposite way when I play around with it. The modal reacts to AnvilSignatureFrame. Whatever width/height you set the frame to, that's how big the modal will be.

How does it respond in mobile? The view units thing is good except when the modal gets below like 4 or 500px. Below that, it should take up the whole width of the screen, or maybe minus like 10px so people know its in a modal.

I'm not opposed to the css file in this component then nulling out the style on the frame. It would let you do media queries, and we could get exactly the behavior we want.

In that case I'll add some CSS for media queries for mobile.

README.md

benogle · 2020-11-19T19:38:54Z

Looking good. I tried it out on the example repo and the styling on the modal is good too 👍

- @anvilco/[email protected] - @anvilco/[email protected]

Winggo · 2020-11-24T21:11:46Z

Addressed all the feedback here and released them in v1.4.4.

benogle

Looks good, maybe just dont need to duplicate the frame default styling

benogle · 2020-11-30T23:01:37Z

packages/react-signature-modal/src/index.js

+          style={enableDefaultStyles
+            ? {
+                width: '80vw',
+                height: '85vh',
+                maxWidth: '1200px',
+                borderStyle: 'none',
+              }
+            : undefined}


I think you can just pass enableDefaultStyles to AnvilSignatureFrame here...

Added this here because the default borderStyle of the frame is groove. The groove borderStyle doesn't look good on the modal so here I've overridden it with borderStyle none.

- @anvilco/[email protected] - @anvilco/[email protected]

Winggo · 2020-12-03T01:14:14Z

packages/react-signature-modal/src/index.css

+@media only screen and (max-width: 500px) {
+  #anvil-signature-modal {
+    width: calc(100vw - 10px);
+    height: 90vh;
+  }


Added css here for small mobile devices mentioned earlier

- @anvilco/[email protected] - @anvilco/[email protected]

benogle · 2020-12-03T23:31:19Z

packages/react-signature-modal/src/index.js

-ReactModal.setAppElement('#root')
+import AnvilSignatureFrame from '../../react-signature-frame/src/index.js'
+import IconClose from './components/IconClose.js'
+import './styles.css'


Didnt we agree to leave the import out of here, but generate the css into dist, then let people import it on their own? I think that would be the most flexible approach

We did agree on that. This generates a styles.css file into dist, and lets people import it if they choose. I can leave the import './styles.css' in the component file since MiniCssExctractPlugin does that for me. The plugin does what you're saying when it sees css imports.

Also tested this on the example app. When I remove import '@anvilco/react-signature-modal/dist/styles.css' the style tag in is removed.

ah ok cool. I thought it injected it. Carry on

Good on making sure I had it right 😉

final touches on docs

489be6b

Winggo added the documentation Improvements or additions to documentation label Nov 17, 2020

Winggo requested a review from benogle November 17, 2020 01:30

Winggo self-assigned this Nov 17, 2020

Winggo added 5 commits November 17, 2020 18:00

review changes

c12e43b

update to docs

5a83923

add react-signature-frame as a dependency rather than a devDependency

46fd52c

add bootstrap step

861616f

try import frame with relative path

872339a

Winggo changed the title ~~Final Touches on Repo Setup and Docs~~ Review Follow Ups and Refactoring Nov 18, 2020

Winggo added 4 commits November 18, 2020 14:00

convert to class components

7801184

update react v16.8 to v16.0

ffd6742

remove react v16.8 req

e4d62a1

Publish

c6e715d

- @anvilco/[email protected] - @anvilco/[email protected]

Winggo added the enhancement New feature or request label Nov 18, 2020

benogle reviewed Nov 19, 2020

View reviewed changes

README.md Show resolved Hide resolved

README.md Show resolved Hide resolved

README.md Outdated Show resolved Hide resolved

Winggo added 2 commits November 24, 2020 12:40

component updates from review

309d070

Publish

764437b

- @anvilco/[email protected] - @anvilco/[email protected]

Winggo requested a review from benogle November 30, 2020 17:13

benogle approved these changes Nov 30, 2020

View reviewed changes

Winggo added 4 commits December 2, 2020 11:54

signatureFrame should have 'withinIframe' query param in signURL

d6268cf

Update index.js

050ffcc

Publish

e6dcf2a

- @anvilco/[email protected] - @anvilco/[email protected]

add css media queries to modal

3f12aed

Winggo commented Dec 3, 2020

View reviewed changes

Winggo added 2 commits December 3, 2020 10:46

extract css file for modal component

807ed9d

miniCssExtractPlugin should output styles.css

06643ea

Winggo added 3 commits December 3, 2020 11:10

remove enableDefaultStyles from modal component

7a8bdbb

update docs for styling

4c88290

Publish

ffdb27b

- @anvilco/[email protected] - @anvilco/[email protected]

Winggo merged commit c904853 into main Dec 3, 2020

Winggo deleted the wt/docs branch December 3, 2020 19:26

benogle reviewed Dec 3, 2020

View reviewed changes

Review Follow Ups and Refactoring #2

Review Follow Ups and Refactoring #2

Uh oh!

Conversation

Winggo commented Nov 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description of the change

Type of change

Related issues

Dev Checklist

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

benogle commented Nov 19, 2020

Uh oh!

Winggo commented Nov 24, 2020

Uh oh!

benogle left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Winggo Dec 4, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Winggo commented Nov 17, 2020 •

edited

Loading

Winggo Dec 4, 2020 •

edited

Loading