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

Add Share Sketch Button #279

Merged
merged 4 commits into from
May 25, 2020
Merged

Add Share Sketch Button #279

merged 4 commits into from
May 25, 2020

Conversation

mattxwang
Copy link
Collaborator

@mattxwang mattxwang commented May 19, 2020

This PR adds a "Share Sketch" button in all TextEditor, which opens a modal (a new component called ShareSketchModal) that allows the user to view the view-only URL to the current program, as well as copy-to-clipboard with a convenient button. I also added a library file sketch.js, which has a function called constructShareableSketchURL that generates the proper URL based on the Sketch ID and environment parameters. Note that this feature works whether or not the user is logged in, and/or viewing their own sketch.

Changes:

  • created new component, ShareSketchModal in src/components/TextEditor/components
  • created new library file, sketch.js in src/lib/, with function constructShareableSketchURL
  • wrote tests for constructShareableSketchURL
  • created a "standard" .scss file for Modal styles, in Modals.scss
  • added button to header of TextEditor to trigger the modal open

Things to note:

  • To implement the copy-to-clipboard, I used the Clipboard.writeText() library. Note that this has no IE support; I have not implemented a fall-back, but I do create an error message.
  • I have not written any tests for ShareSketchModal. I think eventually we should do a modal standardization pass + a set of smoke tests we can use.
  • I think the button header is pretty stacked already, and with fork coming soon it'll be more crowded. We should figure out some way to make it more compact (either a dropdown or smaller icons), but that's out of scope of this PR.

@mattxwang mattxwang marked this pull request as draft May 19, 2020 02:31
@mattxwang mattxwang marked this pull request as ready for review May 23, 2020 10:29
@mattxwang mattxwang requested a review from krashanoff May 23, 2020 10:37
@mattxwang mattxwang added the feature New feature or request label May 23, 2020
Copy link
Contributor

@krashanoff krashanoff left a comment

Choose a reason for hiding this comment

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

Hey Matt, code is looking good and functionality is there. Thanks a ton! I have no issue with merging this in its current state, but I do think it's possible to write a test for constructShareableSketchURL - I might very well be wrong though.

I have not yet written any tests for constructShareableSketchURL. Not entirely sure how we can mock window.location.hostname, but any suggestions are welcome. In theory, I could make this a pure function, but I think that actually leads to more code dup.

If I remember correctly I think you can just go and set it. Not sure this would play nice with our testing setup though.

...I have not written any tests for ShareSketchModal. I think eventually we should do a modal standardization pass + a set of smoke tests we can use.

I agree. This is another PR laying in wait - added to the board.

I think the button header is pretty stacked already, and with fork coming soon it'll be more crowded. We should figure out some way to make it more compact (either a dropdown or smaller icons), but that's out of scope of this PR.

This was the first thing that I noticed actually, checking this out 😅. I think that a button header redesign is in order, and I have an idea of what might work for it. Here's an idea from someone who obviously does not do a lot of design work:
image

jest stub mocks were probably overkill, huh
@mattxwang
Copy link
Collaborator Author

Cool, I ended up modifying the constructShareableSketchURL function to take an optional param that defaults to window.location.hostname, which makes testing easier. Also added a somewhat sketchy mock that just deletes the existing object and re-instantiates it: documentation about mocking variables (opposed to methods) is a bit shaky. Then, wrote some tests for the function.

LMK if it needs any other changes!

@mattxwang mattxwang requested a review from krashanoff May 25, 2020 07:26
Copy link
Contributor

@krashanoff krashanoff left a comment

Choose a reason for hiding this comment

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

Great work Matt! I won't lie that the way we test with window.location is less than ideal; when an update breaks this we will have to find another way to get around this. Awesome idea on the standardization pass on our modals, by the way - that'll be another PR for another day, though.


describe("constructShareableSketchURL", () => {
it("works with localhost", () => {
expect(constructShareableSketchURL("owo", "localhost")).toBe("http://localhost:8080/p/owo");
Copy link
Contributor

Choose a reason for hiding this comment

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

owo

@krashanoff krashanoff merged commit 8fe2e6e into master May 25, 2020
@krashanoff krashanoff deleted the add-share-button branch May 25, 2020 19:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants