Skip to content

Conversation

@axshaykp
Copy link

@axshaykp axshaykp commented Sep 29, 2024

Before submitting your pull request

  • I agree to license my code under the MPL 2.0 license.
  • I rebased my work on top of the main branch.
  • I ran npm test and all tests passed.
  • I added test coverages if relevant.

Description

Duplicate entries in the previous panel path were preventing the back button from navigating to the previous panel, which hindered container editing.

Type of change

Select all that apply.

  • Bug fix
  • New feature
  • Major change (fix or feature that would cause existing functionality to work differently than in the current version)

Tag issues related to this pull request:

@axshaykp axshaykp changed the title Fix #2437: The current panel is not added to the history if it’s alre… Fix #2437: The current panel will not be added to the history if it’s already the last panel Oct 7, 2024
@dannycolin dannycolin self-requested a review October 10, 2024 18:21
@dannycolin dannycolin requested a review from bakulf October 19, 2024 16:06

async showPanel(panel, currentIdentity = null, backwards = false, addToPreviousPanelPath = true) {
if ((!backwards && addToPreviousPanelPath) || !this._currentPanel) {
if ((!backwards && addToPreviousPanelPath) && this._currentPanel && this._currentPanel !== panel) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

While the change seems to work in this specific step-to-reproduce, it introduces an issue in the conditional logic in the following scenario:

  1. Open popup to show containerList
  2. Click on the right arrow of the Personal container to go to the containerInfo panel
  3. In containerInfo panel, click on "manage this container" button
  4. In containerEdit panel, click on the back button to go to the previous panel containerInfo
  5. In containerInfo panel, click on the back button to go to the previous panel containerList

On the last step, backwards value is set to false but it should be set to true since we clicked on the back button.

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.

2 participants