Add Faithlife social share to share links to faithlife, email, facebook and twitter #50
Conversation
|
|
||
| // Radium automatically prefixes radiumConfig with _ | ||
| // eslint-disable-next-line no-underscore-dangle | ||
| const config = this.context._radiumConfig; |
There was a problem hiding this comment.
styled-ui doesn't use Radium, so the user agent will have to be obtained through navigator.userAgent. Note that navigator is not defined when SSR so special care will need to be taken if accessing here.
There was a problem hiding this comment.
thanks. i'm unfamiliar with Radium so I tried to remove the usages but missed this one
| this.setState({ showFeedback: true }); | ||
|
|
||
| if (!this.feedbackTimer) { | ||
| this.feedbackTimer = setTimeout(() => { |
There was a problem hiding this comment.
I prefer to use a debounced method instead of maintaining a timer in these situations. I find it easier to read and reason about. Something like:
componentWillUnmount() {
this.hideFeedbackSoon.cancel();
}
hideFeedbackSoon = debounce(() => {
this.setState({ showFeedback: false });
}, 2000);
copyToClipboard = () => {
...
this.hideFeedbackSoon();
...
};
There was a problem hiding this comment.
Seems like a good fit for lodash.debounce
| import { Button } from '../button/component.jsx'; | ||
| // import { isIOSDevice } from '../../helpers/user-agent-helper'; | ||
|
|
||
| export class CopyToClipboardButton extends React.Component { |
There was a problem hiding this comment.
I'm wondering if we should use a more rounded copy library instead of user agent sniffing to try to determine browser support for copying. Maybe clipboard.js? Also, it would be awesome to export this component from styled-ui as well as the ShareDialog.
There was a problem hiding this comment.
I shall investigate this :)
components/input/component.jsx
Outdated
| import { forwardClassRef } from '../utils/forwardref-wrapper.jsx'; | ||
| import * as Styled from './styled.jsx'; | ||
|
|
||
| export const Input = forwardClassRef( |
There was a problem hiding this comment.
@dustinsoftware I added an input, it's pretty simple and I haven't tested it fully yet because I still can't get a ref pointing at it. Could you take a look at this in the am?
|
@ianfisk can you take another look at this pr? |
| const encodedMessage = message ? encodeURIComponent(message) : ''; | ||
|
|
||
| return ( | ||
| <Modal renderFooter={() => null} isOpen={isOpen} onClose={onClose} title="Share this page"> |
There was a problem hiding this comment.
Creating a new function for every render here may result in unnecessary updating.
There was a problem hiding this comment.
The proper solution is probably to add a hideFooter to Modal instead of just passing in a null function? I didn't do that because I wasn't sure so maybe @dustinsoftware can chime in here?
There was a problem hiding this comment.
A withoutFooter prop was added for this purpose very recently.
https://faithlife.github.io/styled-ui/#/modal/documentation
There was a problem hiding this comment.
hey I somehow missed that... thanks @awesomebob
|
|
||
| export class CopyToClipboard extends React.Component { | ||
| static propTypes = { | ||
| clipboard: PropTypes.func.isRequired, |
There was a problem hiding this comment.
Accepting clipboard as a prop feels unnecessary. I think the fact we are using Clipboard.js is an implementation detail of this component that we don't need to expose like this.
| const input = this.input.current; | ||
|
|
||
| const Clipboard = this.props.clipboard; | ||
| this.clipboard = new Clipboard(button); |
There was a problem hiding this comment.
This instance needs to be cleaned up in componentWillUnmount:
componentWillUnmount() {
this.clipboard.destroy();
}
|
|
||
| constructor(props) { | ||
| super(props); | ||
| this.input = React.createRef(); |
There was a problem hiding this comment.
This is a bit of a NIT, but I prefer using class fields instead of defining instance variables in a constructor.
There was a problem hiding this comment.
Why? Remove the need for the constructor?
There was a problem hiding this comment.
Mainly because class fields are new and shiny and it requires less typing. It would also be more consistent with almost all components in styled-ui.
…ok and twitter. Not complete: iframing and tabs. Mobile ui and copy button is larger than input.
Included: Copy works Not included: Currently copy doesn't select the text, need to add a check for error and a check for if clipboard isn't supported, I think this will require adding a `ref` to the input which I haven't figured out how to do yet.
AuresaNyctea
force-pushed
the
social-share
branch
from
38888fe to
c8cf13c
Compare
|
Build is passing. @dustinsoftware without objection, I will merge and publish this PR today. |
Not complete: iframing and tabs. Mobile ui and copy button is larger than input.