Make listing details modal close on internally-triggered navigation

[fyodorio]

What type of PR is this? (check all applicable)

• Refactor
• Feature
• Bug Fix
• Optimization
• Documentation Update

Description

The detailed description is here but in general, when you (1) go to the listings page and (2) open one of the listings by clicking its title, and then (3) try to navigate from it to the specific category this listing belongs to (by clicking category name in the listing modal footer), (4) navigation happens on the background but (5) modal window doesn't close (though it should).

The solution is simple - we can check if a modal is open in selectCategory() handler in listings.jsx and if it is - close the modal.

Ideally it would probably be better to hook up on navigation event inside the Modal component itself, but that's not how it works now everywhere else in the code so I would suggest to stick with this (more specific) solution.

Related Tickets & Documents

CLOSES #14459

QA Instructions, Screenshots, Recordings

Steps to reproduce are described in the issue details 👆

Added/updated tests?

• Yes
• No, and this is why
• I need help with writing tests

[Forem core team only] How will this change be communicated?

Will this PR introduce a change that impacts Forem members or creators, the
development process, or any of our internal teams? If so, please note how you
will share this change with the people who need to know about it.

• I've updated the Developer Docs and/or
  Admin Guide, or
  Storybook (for Crayons components)
• I've updated the README or added inline documentation
• I've added an entry to
  CHANGELOG.md
• I will share this change in a Changelog
  or in a forem.dev post
• I will share this change internally with the appropriate teams
• I'm not sure how best to communicate this change and need help
• This change does not need to be communicated, and this is why not: please
  replace this line with details on why this change doesn't need to be
  shared

[optional] What gif best describes this PR or how it makes you feel?

[CLAassistant]

All committers have signed the CLA.

[github-actions]

Thank you for opening this PR! We appreciate you!

For all pull requests coming from third-party forks we will need to
review the PR before we can process it through our CI pipelines.

A Forem Team member will review this contribution and get back to
you as soon as possible!

[rhymes]

Works perfectly, thank you @fyodorio!

[rhymes]

@fyodorio for some reason Travis was stuck, so I added an empty commit to unstuck it :D

[msarit]

@nickytonline @aitchiss Hey Nick and Suzanne, could you weigh in on the addition of a test for this PR? The contributor had this to say:

I'd tried to create a test file using app/javascript/listings/__tests__/ListingDashboard.test.jsx as an example to write some kind of an integration test for the scenario, but wasn't able to compile the container correctly. I suppose I'm doing something wrong with regards to the component being server-rendered (as far as I understand). I would be happy to add the test (if necessary) if someone would be able to help me with scaffolding the suite for it.

[aitchiss]

I think this change would be a great fit for a Cypress integration test - you can check this section in our docs for full details of how we use Cypress & some best practice tips.

This could be a new file in the directory cypress/integration/seededFlows/listingFlows/, and you can see an example of a Cypress test opening up a mocked listing in cypress/integration/seededFlows/listingFlows/viewListing.spec.js:

forem/cypress/integration/seededFlows/listingFlows/viewListing.spec.js

Lines 23 to 44 in 4858b75

	it('opens a listing from the listings page', () => {
	cy.intercept(
	'/search/listings?category=&listing_search=&page=0&per_page=75&tag_boolean_mode=all',
	{ fixture: 'search/listings.json' },
	);
	cy.visit('/listings');
	cy.findByRole('link', { name: 'Listing title' }).as('listingTitle');
	cy.get('@listingTitle').click();
	cy.findByTestId('listings-modal').as('listingsModal');
	cy.get('@listingsModal').findByText('Listing').should('exist');
	cy.get('@listingsModal').findByText('Listing title').should('exist');
	cy.get('@listingsModal')
	.findAllByRole('button')
	.first()
	.should('have.focus');
	cy.get('@listingsModal').findByRole('button', { name: /Close/ }).click();
	cy.get('@listingTitle').should('have.focus');
	});

I'm thinking your test could be very similar the one above, but instead of closing the open modal, selecting a link from inside it and then checking the modal is no longer visible.

Let me know how you get on and if you have any difficulties 👍

[fyodorio]

@aitchiss thanks for the guidance Suzanne, sounds good, will write the test and update the PR 👌

[fyodorio]

@aitchiss @rhymes @msarit @Ridhwana I have just added the e2e test case, please take a look

[cmgorton]

@fyodorio thanks again for working on this bug. We are wrapping up the bug smash event and I wanted to encourage you to write a post about your experience if you have time.
You can use this template.
https://dev.to/new/devbugsmash

[fyodorio]

@fyodorio thanks again for working on this bug. We are wrapping up the bug smash event and I wanted to encourage you to write a post about your experience if you have time.
You can use this template.
https://dev.to/new/devbugsmash

Done, @cmgorton, sorry that it's quite late, busy September 😅