search

WordPress / wporg-theme-directory

12 stars 6 forks source link

Theme previewer: Update back button to navigate all the way back to the theme details page. #114

Closed ryelle closed 1 month ago

ryelle commented 1 month ago

Fixes #87. In that issue, it was suggested that the variations and patterns not add history entries. Unfortunately, the navigation of the iframe causes the entries, so we can't get around that. Instead, I've tried a different method. Now, the code tracks how many variations & patterns you've selected, and then uses that count to go backwards X times so that you're back at the theme details page.

If the previewer was opened in a new window, it realizes the history is too short, and just falls back to a regular link to the details view.

Currently, this does not handle navigating inside the iframe, for example if you click a post link, it will throw off the history counter. If this approach sounds good I can look into fixing this.

Screenshots

https://github.com/WordPress/wporg-theme-directory/assets/541093/147ec4f5-36c7-4c3a-9757-96d81d8111c5

How to test the changes in this Pull Request:

  1. View a theme archive
  2. Open the previewer (in the same tab)
  3. Click around variations and patterns
  4. Click the previewer back button, it should take you to the theme details page
  5. Click your browser's back button, it should take you back to the theme archive you came from
fcoveram commented 1 month ago
  1. Click your browser's back button, it should take you back to the theme archive you came from

If I understand correctly, and based on the video attached, this step takes you to the archive page "Author: WordPress.org" where 4 themes are list, right? If so, is it possible to go back to the theme details page as in step 4?

From the quote below I understood that it is possible. But I could have got it wrong.

…Now, the code tracks how many variations & patterns you've selected, and then uses that count to go backwards X times so that you're back at the theme details page.

ryelle commented 1 month ago

If I understand correctly, and based on the video attached, this step takes you to the archive page "Author: WordPress.org" where 4 themes are list, right? If so, is it possible to go back to the theme details page as in step 4?

Correct, step 5 goes back to the page before the theme details, in this case, the author archive. You can think about the history like a stack of cards, we're just navigating back and forward through them. Steps 1-3 create the stack (I missed one step, 1.5, click a theme to view the detail page), and then steps 4 and 5 navigate back through them. In step 4, we jump all the way back to the details page. Then in step 5 we go back once more, to get to the author archive. From here, you could use the browser forward button to "get back" to the detail view, moving up the stack. Or you could click a different theme, restarting the stack.

history

fcoveram commented 1 month ago

Very clear. I like how it works and don't envision confusing flows ✨ 🚀

ndiego commented 1 month ago

This flow looks great 🤩

StevenDufresne commented 1 month ago

I can get the state out of sync using the forward button.

Test case:

Video: https://d.pr/i/qwuOfq

ryelle commented 1 month ago

I can get the state out of sync using the forward button.

😩 yeah… I noticed that later too, but I can't figure out a way to detect "forward" navigation. Do you have any suggestions?

ryelle commented 1 month ago

Caught back up on my work from Friday and I think I actually did figure this one out— the change in https://github.com/WordPress/wporg-theme-directory/pull/114/commits/e11111755bede7b5dcfb55905c0455b258ed7f15 should work even if the user clicks browser back/forward, because it tracks the difference in total history entries. IIRC, I didn't push it on Friday because I found another edge case, but I've just tested it again and it seems to work. @StevenDufresne could you give this update a try?

ndiego commented 1 month ago

@ryelle is it possible to test the changes on the live site, or only in a testing environment?

ryelle commented 1 month ago

Not without merging it, and at this point I'd rather not merge until we know it's workable.

ndiego commented 1 month ago

Not without merging it, and at this point I'd rather not merge until we know it's workable.

Yeah, sounds good.

StevenDufresne commented 1 month ago

This isn't working for me, it appears like in Chrome replaceState creates an entry. I can confirm that it does work in FireFox.

ryelle commented 1 month ago

This isn't working for me, it appears like in Chrome replaceState creates an entry. I can confirm that it does work in FireFox.

replaceState shouldn't create an entry, but the iframe navigation event does, and tracking history.length accounts for that. I'm not sure what bug you ran into. Either way, I'm able to get into a weird spot by clicking the browser back button, then clicking the app back button — it pulls me back one too many pages (on Chrome and FF).

It sounds like this is just a limit of the current history API — you can't read the history entries, or determine where in the stack you are. In the (maybe near) future we should get a cross-browser Navigation API which should help, but it's not implemented in FF or Safari yet.

Given that, I'm inclined to close this PR and leave the current behavior (a normal link) for the Back button. Maybe we can revisit this in a couple months if the Navigation API becomes available, or maybe updating the previewer will also change how we do this navigation.