Page MenuHomeSoftware Heritage
Differential D7426

Maintain tab selection on refresh
ClosedPublic
Actions

Authored by jayeshv on Mar 24 2022, 6:23 PM.

Details

Reviewers
anlambert
Group Reviewers
Reviewers
Maniphest Tasks
T4050: [Add forge now] Refreshing the page loses the current tab
Commits
rDWAPPS6f1a974c1f4c: Maintain tab selection on refresh
Summary

Selected tab is maintained in add-forge-now create view.

Related to T4050

Diff Detail

Repository
rDWAPPS Web applications
Branch
tab-fix
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 27794
Build 43506: Phabricator diff pipeline on jenkinsJenkins console · Jenkins
Build 43505: arc lint + arc unit

Event Timeline

jayeshv created this revision.Mar 24 2022, 6:23 PM
Herald added a reviewer: Reviewers. · View Herald TranscriptMar 24 2022, 6:23 PM
jayeshv edited the summary of this revision. (Show Details)Mar 24 2022, 6:24 PM
jayeshv added a task: T4050: [Add forge now] Refreshing the page loses the current tab.
jayeshv updated this revision to Diff 26876.Mar 24 2022, 6:32 PM

Commit message change

swh-public-ci added a comment.Mar 24 2022, 6:41 PM

Build is green

Patch application report for D7426 (id=26875)

Rebasing onto 7c86b0e45c...

Current branch diff-target is up to date.
Changes applied before test
commit 7c292a0a20ee282fd0c9800b376c3f4513229312
Author: Jayesh Velayudhan <jayesh@softwareheritage.org>
Date:   Thu Mar 24 18:16:41 2022 +0100

    Maintain tab selection on refresh
    
    Selected tab is maintained in add-forge-now create view.
    
    Related to T4050

See https://jenkins.softwareheritage.org/job/DWAPPS/job/tests-on-diff/1565/ for more details.

Harbormaster completed remote builds in B27788: Diff 26875.Mar 24 2022, 6:41 PM
swh-public-ci added a comment.Mar 24 2022, 6:51 PM

Build is green

Patch application report for D7426 (id=26876)

Rebasing onto 7c86b0e45c...

Current branch diff-target is up to date.
Changes applied before test
commit d25a0cc69b81104cc6eeb759494b1df4fc78c278
Author: Jayesh Velayudhan <jayesh@softwareheritage.org>
Date:   Thu Mar 24 18:29:55 2022 +0100

    Maintain tab selection on refresh
    
    Maintain selected in add-forge-now create view.
    Add URL hash for the help tab.
    
    Related to T4050

See https://jenkins.softwareheritage.org/job/DWAPPS/job/tests-on-diff/1566/ for more details.

Harbormaster completed remote builds in B27789: Diff 26876.Mar 24 2022, 6:51 PM
jayeshv requested review of this revision.Mar 24 2022, 6:51 PM
anlambert requested changes to this revision.Mar 24 2022, 7:08 PM
anlambert added a subscriber: anlambert.

Could you add a cypress test for this ? You can inspire from the one I wrote for save code now.

assets/src/bundles/add_forge/create-request.js
66

Why triggering the same event again ? This should not be needed.

This revision now requires changes to proceed.Mar 24 2022, 7:08 PM
jayeshv added inline comments.Mar 25 2022, 10:11 AM
assets/src/bundles/add_forge/create-request.js
66

It is not working on page refresh otherwise.

jayeshv updated this revision to Diff 26881.Mar 25 2022, 10:25 AM

Added extra cypress tests

assets/src/bundles/add_forge/create-request.js
66

This seems to be the case here
https://stackoverflow.com/questions/20652020/the-hashchange-event-of-jquery-doesnt-work-if-i-open-a-page-with-hash-directly

swh-public-ci added a comment.Mar 25 2022, 10:43 AM

Build is green

Patch application report for D7426 (id=26881)

Rebasing onto d5e3f98417...

Current branch diff-target is up to date.
Changes applied before test
commit 0e103975733b908b92fed170a75a378a682d99a6
Author: Jayesh Velayudhan <jayesh@softwareheritage.org>
Date:   Fri Mar 25 10:23:10 2022 +0100

    Maintain tab selection on refresh
    
    Maintain selected tab in add-forge-now create view.
    Add URL hash for the help tab.
    Improve cypress tests for the tab change.
    
    Related to T4050

See https://jenkins.softwareheritage.org/job/DWAPPS/job/tests-on-diff/1570/ for more details.

Harbormaster completed remote builds in B27794: Diff 26881.Mar 25 2022, 10:43 AM
anlambert requested changes to this revision.Mar 25 2022, 11:07 AM
anlambert added inline comments.
assets/src/bundles/add_forge/create-request.js
66

I would rather put the the code inside the hashchange event handler in a function and call it at the end of the onCreateRequestPageLoad function instead.
The current code gives me the impression that this will loop forever when the hash changes.

This revision now requires changes to proceed.Mar 25 2022, 11:07 AM
jayeshv updated this revision to Diff 26889.Mar 25 2022, 11:30 AM

addressing review comments

jayeshv marked an inline comment as done.Mar 25 2022, 11:30 AM
anlambert requested changes to this revision.Mar 25 2022, 11:35 AM

You still need to keep the hashchange event handler or the tabs will not be switched when playing with browser history:

$(window).on('hashchange', () => {
  onPageHashChage();
});
This revision now requires changes to proceed.Mar 25 2022, 11:35 AM
jayeshv updated this revision to Diff 26890.Mar 25 2022, 11:43 AM

review comment; Bug fix for browser history

swh-public-ci added a comment.Mar 25 2022, 11:49 AM

Build is green

Patch application report for D7426 (id=26889)

Rebasing onto 349c7e5f4e...

Current branch diff-target is up to date.
Changes applied before test
commit b81477d5fc0ae91b1cfd6656d734c7ce086bcb60
Author: Jayesh Velayudhan <jayesh@softwareheritage.org>
Date:   Fri Mar 25 11:29:13 2022 +0100

    Maintain tab selection on refresh
    
    Maintain selected tab in add-forge-now create view.
    Add URL hash for the help tab.
    Improve cypress tests for the tab change.
    
    Related to T4050

See https://jenkins.softwareheritage.org/job/DWAPPS/job/tests-on-diff/1576/ for more details.

Harbormaster completed remote builds in B27802: Diff 26889.Mar 25 2022, 11:49 AM
swh-public-ci added a comment.Mar 25 2022, 12:03 PM

Build is green

Patch application report for D7426 (id=26890)

Rebasing onto 8d5f258e31...

First, rewinding head to replay your work on top of it...
Applying: Maintain tab selection on refresh
Changes applied before test
commit 5348f121624b8617c0923c7de38286e7fae83bdc
Author: Jayesh Velayudhan <jayesh@softwareheritage.org>
Date:   Fri Mar 25 11:42:11 2022 +0100

    Maintain tab selection on refresh
    
    Maintain selected tab in add-forge-now create view.
    Add URL hash for the help tab.
    Improve cypress tests for the tab change.
    
    Related to T4050

See https://jenkins.softwareheritage.org/job/DWAPPS/job/tests-on-diff/1577/ for more details.

Harbormaster completed remote builds in B27804: Diff 26890.Mar 25 2022, 12:03 PM
anlambert accepted this revision.Mar 25 2022, 3:27 PM
This revision is now accepted and ready to land.Mar 25 2022, 3:27 PM
jayeshv updated this revision to Diff 26909.Mar 25 2022, 3:31 PM

rebase

This revision was landed with ongoing or failed builds.Mar 25 2022, 3:32 PM
Closed by commit rDWAPPS6f1a974c1f4c: Maintain tab selection on refresh (authored by jayeshv). · Explain Why
This revision was automatically updated to reflect the committed changes.
jayeshv added a commit: rDWAPPS6f1a974c1f4c: Maintain tab selection on refresh.
swh-public-ci added a comment.Mar 25 2022, 3:54 PM

Build was aborted

Patch application report for D7426 (id=26909)

Rebasing onto cdda03c3aa...

Current branch diff-target is up to date.
Changes applied before test
commit 6f1a974c1f4c4fe78ca14ed5f1766bbd510a8cea
Author: Jayesh Velayudhan <jayesh@softwareheritage.org>
Date:   Fri Mar 25 11:42:11 2022 +0100

    Maintain tab selection on refresh
    
    Maintain selected tab in add-forge-now create view.
    Add URL hash for the help tab.
    Improve cypress tests for the tab change.
    
    Related to T4050

Link to build: https://jenkins.softwareheritage.org/job/DWAPPS/job/tests-on-diff/1587/
See console output for more information: https://jenkins.softwareheritage.org/job/DWAPPS/job/tests-on-diff/1587/console

Harbormaster failed remote builds in B27822: Diff 26909!Mar 25 2022, 3:54 PM

Revision Contents
Changeset List

PathSizeCoverage (All)Coverage (Touched)
assets/
src/
bundles/
add_forge/
8 lines95%
Loading...
cypress/
integration/
20 lines--

Diff 26881

View Options

assets/src/bundles/add_forge/create-request.js

Loading...
View Options

cypress/integration/add-forge-now-request-create.spec.js

Loading...
About · Developer information · Legal