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

Share modal #419

Merged
merged 6 commits into from
May 7, 2021
Merged

Share modal #419

merged 6 commits into from
May 7, 2021

Conversation

macfarlandian
Copy link
Collaborator

@macfarlandian macfarlandian commented May 6, 2021

Description of the change

Includes a modal in the main navigation with buttons for copying the page URL or sharing it directly to email, facebook, or twitter. Including the section number is optional.

Screen Shot 2021-05-05 at 5 08 13 PM
(it will reflect whatever the current host is, which is why it's just showing my local dev URL)

It's possible we might add more copy to the modal or the links besides just the URL, but no one has given me anything yet so I didn't want to continue waiting for it. Wouldn't change the basic functionality anyway so easy enough to add later if needed.

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)
  • Configuration change (adjusts configuration to achieve some end related to functionality, development, performance, or security)

Related issues

Closes #348

Checklists

Development

These boxes should be checked by the submitter prior to merging:

  • Manual testing against realistic data has been performed locally

Code review

These boxes should be checked by reviewers prior to merging:

  • This pull request has a descriptive title and information useful to a reviewer
  • This pull request has been moved out of a Draft state, has no "Work In Progress" label, and has assigned reviewers
  • Potential security implications or infrastructural changes have been considered, if relevant

@coveralls
Copy link

coveralls commented May 6, 2021

Pull Request Test Coverage Report for Build 818733565

  • 38 of 41 (92.68%) changed or added relevant lines in 7 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.3%) to 73.664%

Changes Missing Coverage Covered Lines Changed/Added Lines %
spotlight-client/src/SiteNavigation/SiteNavigationContainer.tsx 3 4 75.0%
spotlight-client/src/ShareModal/ShareModal.tsx 23 25 92.0%
Totals Coverage Status
Change from base Build 814504662: 0.3%
Covered Lines: 2870
Relevant Lines: 3769

💛 - Coveralls

Copy link

@jovergaag jovergaag left a comment

Choose a reason for hiding this comment

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

Couple of comments but nothing show stopping!

<UrlText>{urlToShare}</UrlText>
<ShareButtons>
<Button onClick={() => copyUrl()}>Copy URL</Button>
<EmailShareButton url={urlToShare}>

Choose a reason for hiding this comment

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

Is there a setting to have this open in a new tab? After sending the email it closes the tab so I lose the dashboard completely.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

unfortunately there is not, but I guess it would be pretty trivial to just write my own mailto link here instead ... I was just using this because it was already there but that's a good point!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

after digging around some more I found that there actually is a way to do this via the library, and it actually seems to work more consistently than a plain mailto link with target="_blank" despite being slightly kludgy, so that's what I went with!

@@ -0,0 +1,4 @@
<svg width="14" height="14" viewBox="0 0 14 14" fill="none" xmlns="http://www.w3.org/2000/svg">

Choose a reason for hiding this comment

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

Are these things we would want to include in the component library?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yeah, probably? I should do a pass to move all of the icons in this application over to that package (if they're not there already) but I didn't want to block on that here.

@macfarlandian macfarlandian merged commit 26d09f1 into main May 7, 2021
@macfarlandian macfarlandian deleted the ian/348-share-modal branch May 7, 2021 01:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Share modal
3 participants