Skip to content

Conversation

@AndrewR8
Copy link
Contributor

Issue URL: #753

Description:

Steps to test this PR:

  1. Navigate to http://www.granditaliaperth.co.uk/menus.html and click on the download link for the menu.
  2. The download will start and a new blank tab closes immediately

Copy link
Contributor

@malmstein malmstein left a comment

Choose a reason for hiding this comment

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

Thanks for this! I'd like to see a test around this functionality so I've left a comment for you to take a look.

@cmonfortep cmonfortep assigned subsymbolic and unassigned cmonfortep Apr 27, 2020
@cmonfortep
Copy link
Contributor

@AndrewR8 one question from my side, is this aligned with the proposed solution in #763?

My advice is to put on hold any extra change till the assignee reviews this PR. Just in case the approach changes and current review comments don't apply anymore. 👍

@AndrewR8
Copy link
Contributor Author

AndrewR8 commented Apr 28, 2020

@AndrewR8 one question from my side, is this aligned with the proposed solution in #763?

@cmonfortep It seems to me yes.

@subsymbolic
Copy link
Contributor

subsymbolic commented Apr 30, 2020

Thanks for submitting this @AndrewR8, I'll review it by Friday.

@subsymbolic
Copy link
Contributor

@AndrewR8 thanks for submitting this, it's great to see progress on such a frustrating user bug. There is one issue with this approach however, if multiple tabs are open the dialog appears over the first tab rather than the tab that requested the dialog.

In order to fix this we'll need to coordinate the events happening in the db and make sure the correct tab is selected. Feel free to look into this but it is complex. If we don't hear from you by Friday, we can use the work you have done so far and complete the implementation in-house.

@AndrewR8
Copy link
Contributor Author

AndrewR8 commented May 6, 2020

@subsymbolic
Due to issue #812 and because we don't store the history of opened tabs somewhere there is no way to make sure at this moment that the correct tab is selected when blank tab closes. Looks like there are only two optimal ways to fix it. It seems to me the easier way to resolve it is to fix issue #812. May I fix this issue first in this PR? or open a new one? (If no objection I could add it. The implementation is already completed)

@subsymbolic
Copy link
Contributor

subsymbolic commented Aug 5, 2020

Many thanks for your time on this @AndrewR8 and a big apology that our codebase changed after you had submitted your PR. We have since received and accepted another PR that fixes this in line with our latest changes as it also makes some db updates that will help with upcoming work. Thanks again for submitting this and for your other contributions in the past.

@subsymbolic subsymbolic closed this Aug 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.

4 participants