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

feat: "Copy link" button in Export dialog #366

Merged
merged 2 commits into from
Apr 5, 2020

Conversation

dkozar
Copy link
Contributor

@dkozar dkozar commented Apr 2, 2020

Related issues and PRs

Description

Encourages the user to copy the link and share it via email etc.

Impacted Areas in the application

Export dialog.

Testing

  • Open the export dialog
  • Click "Copy link" button
  • Open a new browser tab and paste a link

copy-link

Video: https://microsofteur-my.sharepoint.com/:i:/g/personal/dakozar_microsoft_com/ES8jveSoVLNAukRCgZ6GCAYBKYsdab8Ll8CTnz8g9QXq6g?e=YnyQ4b

@vercel
Copy link

vercel bot commented Apr 2, 2020

This pull request is being automatically deployed with ZEIT Now (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://zeit.co/covid19-scenarios/covid19-scenarios/4zur6uvmw
✅ Preview: https://covid19-scenarios-git-fork-dkozar-feat-share-link.covid19-scenarios.now.sh

@vercel vercel bot temporarily deployed to Preview April 2, 2020 13:48 Inactive
@dkozar dkozar force-pushed the feat/share-link branch from 003567a to af85980 Compare April 2, 2020 13:50
Comment on lines 57 to 46
<Popover placement={popoverPlacement} open={popoverOpen} target={popoverTarget} ontoggle={closePopover}>
<PopoverHeader>{popoverHeader}</PopoverHeader>
<PopoverBody>{popoverBody}</PopoverBody>
</Popover>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Doesn't show up (but doesn't break the app either 😄 ).

Anyone knows why?

Copy link
Member

@ivan-aksamentov ivan-aksamentov Apr 3, 2020

Choose a reason for hiding this comment

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

Because we are already in a popover? I think this particular implementation is modal, that means there could be only one.

Maybe we could just change the button's text and/or add a green checkmark somewhere? (on successful copy)

@nnoll
Copy link
Collaborator

nnoll commented Apr 3, 2020

This functionality is very much welcome. I've been getting sick of trying to copy our absurdely long URLs. I fully approve merging this but I'll let @ivan-aksamentov take a quick look as it adds a dependency.

@dkozar
Copy link
Contributor Author

dkozar commented Apr 3, 2020

@nnoll as for dependency, it’s for this single line:

popoverPlacement?: Popper.Placement

This is because Popover we’re using internally uses Poper and uses this enum. I could have copied this enum manually, but I think this is more decent since Popper seems to be already bundled into our solution.

Comment on lines 57 to 46
<Popover placement={popoverPlacement} open={popoverOpen} target={popoverTarget} ontoggle={closePopover}>
<PopoverHeader>{popoverHeader}</PopoverHeader>
<PopoverBody>{popoverBody}</PopoverBody>
</Popover>
Copy link
Member

@ivan-aksamentov ivan-aksamentov Apr 3, 2020

Choose a reason for hiding this comment

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

Because we are already in a popover? I think this particular implementation is modal, that means there could be only one.

Maybe we could just change the button's text and/or add a green checkmark somewhere? (on successful copy)

@dkozar
Copy link
Contributor Author

dkozar commented Apr 3, 2020

@ivan-aksamentov so popover in modal isn't doable? I'm googling and seeing others have similar problem. Huh...

@dkozar
Copy link
Contributor Author

dkozar commented Apr 3, 2020

@ivan-aksamentov I decided just to change the button text, as suggested 😃

copied

@dkozar dkozar requested a review from ivan-aksamentov April 3, 2020 22:43
@dkozar
Copy link
Contributor Author

dkozar commented Apr 4, 2020

Could this be merged? The popover has been removed as suggested..

@ivan-aksamentov ivan-aksamentov merged commit fe73ead into neherlab:master Apr 5, 2020
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.

3 participants