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

Jump to bottom

540 - Change external link to open in new tab by default #541

Merged
merged 14 commits into from Jun 26, 2020
Merged

540 - Change external link to open in new tab by default #541

merged 14 commits into from Jun 26, 2020

Conversation

nandanmen
Copy link
Contributor

@nandanmen nandanmen commented Jun 1, 2020 •
edited

Closes #540

Note that all tapestries created prior to this PR will not have the new tab selected by default (e.g. when editing). The warning message however will show regardless.

Testing

  • Create a new node
  • Change the node type to url embed
    • The "Open in a new window" radio button should be selected by default
  • Press the "Embed in Tapestry" option
    • A warning message should appear telling the user that links may not work correctly.

@nandanmen nandanmen added the needs testing Has been tested by author and needs a second pair of eyes to test it label Jun 1, 2020
@nandanmen nandanmen linked an issue Jun 1, 2020 that may be closed by this pull request
Change external link to open in new tab by default #540
Closed
@nandanmen nandanmen requested a review from wynnset June 1, 2020 22:37
@liam-armstrong
Copy link
Contributor

Radio button is selected by default, and warning pops up if you select embed. However, the actual site doesn't pop up in a new window when I click the media button. I'm greeted with this screen:
image Which has a link to the external site (I used https://liam-armstrong.com, hence my name on the modal)

I see you haven't made any changes to the actual new-window behavior in this PR though. Is this a separate issue? If so, the actual changes in this PR are good to go.

@nandanmen
Copy link
Contributor Author

Yes, when we mean by "new window" it means to open the preview of whatever that link will be.

@liam-armstrong
Copy link
Contributor

Ok, in that case this PR is ready to go.

@liam-armstrong liam-armstrong added needs final review Has been tested and reviewed once and needs another code review to become ready for merge and removed needs testing Has been tested by author and needs a second pair of eyes to test it labels Jun 9, 2020
@nandanmen nandanmen changed the title Change external link to open in new tab by default 540 - Change external link to open in new tab by default Jun 16, 2020
Base automatically changed from 531-refactor-node-modal to master June 24, 2020 06:49
@nandanmen nandanmen removed the request for review from wynnset June 24, 2020 16:53
@nandanmen nandanmen self-assigned this Jun 24, 2020
@nandanmen nandanmen added needs work This PR has bugs or not complete as per its requirements and removed needs final review Has been tested and reviewed once and needs another code review to become ready for merge labels Jun 24, 2020
@nandanmen nandanmen requested a review from wynnset June 24, 2020 21:43
@nandanmen nandanmen assigned wynnset and unassigned nandanmen Jun 24, 2020
@nandanmen nandanmen removed the needs work This PR has bugs or not complete as per its requirements label Jun 24, 2020
@nandanmen nandanmen added the needs final review Has been tested and reviewed once and needs another code review to become ready for merge label Jun 24, 2020
@wynnset wynnset merged commit da8775c into master Jun 26, 2020
@wynnset wynnset deleted the 540-change-external-link branch June 26, 2020 03:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wynnset wynnset wynnset approved these changes

Assignees

@wynnset wynnset

Labels
needs final review Has been tested and reviewed once and needs another code review to become ready for merge
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Change external link to open in new tab by default
3 participants
@nandanmen @liam-armstrong @wynnset