C2LC-248: Initial Implementation of share button. #98

the-t-in-rtf · 2020-11-10T12:57:22Z

See C2LC-248 for details.

the-t-in-rtf · 2020-11-10T13:00:21Z

Quick notes on this:

First, I didn't even try to position the share button. I'll merge with upstream once https://issues.fluidproject.org/browse/C2LC-246 is merged and put it next to the speed controls.

Second, I used a Modal for now, I wanted to discuss perhaps using a Toast instead.

Third, obviously I'll add tests, since we can read the clipboard using the same API and confirm that the value was set.

…trols.

the-t-in-rtf · 2020-11-11T14:51:21Z

@simonbates, as discussed I just committed my work in progress on new tests for the ShareButton component. There are two failures I could use help with.

 FAIL  src/ShareButton.test.js
  ● displays modal on click

    TypeError: Cannot read property 'writeText' of undefined

      38 |         // Copy the URL to the clipboard, see:
      39 |         // https://developer.mozilla.org/en-US/docs/Web/API/Clipboard/writeText
    > 40 |         navigator.clipboard.writeText(currentUrl).then(() => {

I made an attempt at mocking that part of the API, but somehow what I did isn't exposed to the component itself, i.e. my tests can access my fake readText method, but the component can't access the similar writeText method.

I also commented out the rendering tests, which fail because I don't seem to have wired up the i18n stuff correctly. I'll commit that as well shortly, but here's the error:

    Invariant Violation: [React Intl] Could not find required `intl` object. <IntlProvider> needs to exist in the component ancestry.

      37 | it('renders without crashing', () => {
      38 |     const div = document.createElement('div');
    > 39 |     ReactDOM.render(<ShareButton intl={intl}/>, div);
         |              ^
      40 |     ReactDOM.unmountComponentAtNode(div);
      41 | });
      42 |

I can break these down and research them on my own, but I figured you might be able to at least point me in the right direction.

- Use the async form of Jest's test() for the tests which click on the Share button, to manage the use of Promises in accessing the clipboard - Add a callback to ShareButton to receive notification when the modal is shown - Update the test names to use Given, When, Then style - Add a FakeClipboard class to test interaction with the clipboard

simonbates · 2020-11-11T22:38:51Z

I've updated the tests for ShareButton in this commit: simonbates@4de7c92

Changes:

Use the async form of Jest's test() for the tests which click on the Share button, to manage the use of Promises in accessing the clipboard
- https://jestjs.io/docs/en/asynchronous
Add a callback to ShareButton to receive notification when the modal is shown
- This was necessary due to the use of Promises: we need to wait until the state has been changed before inspecting it
- I don't love adding a property to ShareButton just to support testing, but it's the best I could think of without making larger changes to the structure of the code
Update the test names to use Given, When, Then style
I packaged your clipboard implementation up into a new class FakeClipboard, to make it easier to use in multiple tests; I also updated the names to match those in the Flow type definition for clipboard: https://github.com/facebook/flow/blob/f4a9d2634bbe7f4b417289d5bcfaaddc481d192d/lib/bom.js#L314-L318

the-t-in-rtf · 2020-11-12T08:54:20Z

Thanks, @simonbates, for the fix and the links. I copied the it style from the rendering tests I was reading through, the other syntax makes more sense to me. Jest has a really wide range of strategies for handling async calls, that's good to know.

I still need to fix up the rendering test, I'll give that a go on my own a bit later today.

simonbates · 2020-11-12T13:53:40Z

src/ShareButton.test.js

+    return wrapper;
+}
+
+// TODO: Figure how to do this properly with the required intl infrastructure


I think we don't need this comment any longer?

simonbates · 2020-11-12T14:02:34Z

src/ShareButton.css

+    margin-top: 1rem;
+    height: 2.25rem;


I think we don't need the margin-top and height properties here.

simonbates · 2020-11-12T14:04:12Z

src/ProgramBlockEditor.js

@@ -41,6 +41,7 @@ type ProgramBlockEditorProps = {

 type ProgramBlockEditorState = {
    showConfirmDeleteAll: boolean,
+    showShareComplete: boolean,


It looks like the changes in ProgramBlockEditor.js are left over from previous work and can be reverted?

I fixed this line and reviewed the changes, what's left now seems appropriate (the new state variable).

simonbates · 2020-11-12T14:05:25Z

src/App.scss

+    background-color: #F1F1F1;
+    display: grid;
+    grid-template-columns: 1fr 1fr 1fr;
+


Extra blank line here

simonbates · 2020-11-12T14:07:17Z

src/ShareButton.js

+
+type ShareButtonProps = {
+    intl: IntlShape,
+    disabled: boolean,


The disabled property is never used.

simonbates · 2020-11-12T14:07:57Z

src/ShareButton.js

+class ShareButton extends React.Component<ShareButtonProps, ShareButtonState> {
+    static defaultProps = {
+        disabled: false,
+        showShareComplete: false


showShareComplete is a state value, rather than props.

C2LC-248: Initial Implementation of share button.

992d830

the-t-in-rtf added 3 commits November 10, 2020 15:36

Merge remote-tracking branch 'upstream/develop-0.5' into C2LC-248

7fa3aa2

C2LC-248: Initial crude integration of share button with playback con…

2759efe

…trols.

Committed work in progress on ShareButton tests for review.

c9d13a1

the-t-in-rtf and others added 4 commits November 11, 2020 15:51

Committed second broken share button test for review.

e11032e

C2LC-248: Committed work in progress on share button layout.

903bfdf

C2LC-248: Commited work in progress on restyling for review.

fd5104d

Merge remote-tracking branch 'simonbates/C2LC-248' into C2LC-248

4fd0e4f

the-t-in-rtf added 2 commits November 12, 2020 09:57

C2LC-248: Fixed rendering test for ShareButton.

9ddae37

C2LC-248: Fixed layout of share button.

e3053e5

simonbates reviewed Nov 12, 2020

View reviewed changes

the-t-in-rtf added 2 commits November 12, 2020 16:08

C2LC-248: Tidied up based on PR feedback.

c429e0a

C2LC-248: Reverted changes to ProgramBlockEditor.

2b85277

simonbates merged commit edf6288 into codelearncreate:develop-0.5 Nov 12, 2020

C2LC-248: Initial Implementation of share button. #98

C2LC-248: Initial Implementation of share button. #98

Uh oh!

Conversation

the-t-in-rtf commented Nov 10, 2020

Uh oh!

the-t-in-rtf commented Nov 10, 2020

Uh oh!

the-t-in-rtf commented Nov 11, 2020

Uh oh!

simonbates commented Nov 11, 2020

Uh oh!

the-t-in-rtf commented Nov 12, 2020

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!

the-t-in-rtf Nov 12, 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!

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!

the-t-in-rtf Nov 12, 2020 •

edited

Loading