feat: adds redirect for 404 how tos #1225

thisislawatts · 2021-10-03T20:08:09Z

PR Checklist

- Latest master branch merged
- PR title descriptive (can be used in release notes)

PR Type

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)

Description

There are many reasons a how to may not be available
on the server. Such as:

Deleted 🔥
Renamed ✏️

In an effort to make the experience less of a dead
end for the visitor we redirect them to the search
page.

This current implementation takes place on the client
side after attempting to load a how-to entry from
the backend data store.

Git Issues

Reference: #1168
Closes #1227

thisislawatts · 2021-10-03T20:09:58Z

cypress/integration/howto/read.spec.ts

+
+  describe('[Fail to find a How-to]', () => {
+    const uuid = crypto.randomBytes(16).toString('hex')
+    const howToNotFoundUrl = `/how-to/this-how-to-does-not-exist-${uuid}`


🤔 Perhaps unneccessary as I think the datastore is completely reset between test runs so there are unlikely to be any collisions due to leaky state between test runs.

It should do (cypress by default clears localstorage and we have manual methods to clear indexeddb), we also use a custom prefix in case of tests running in parallel. That being said, no real harm to include

There are many reasons a how to may not be available on the server. Such as: * Deleted 🔥 * Renamed ✏️ In an effort to make the experience less of a dead end for the visitor we redirect them to the search page. This current implementation takes place on the client side after attempting to load a how-to entry from the backend data store. Reference: #1168

github-actions · 2021-10-03T20:29:08Z

Visit the preview URL for this PR (updated for commit ab03241):

https://onearmy-next--pr1225-feat-adds-redirect-f-38hqowj0.web.app

_{(expires Thu, 04 Nov 2021 19:35:18 GMT)}

_{🔥 via Firebase Hosting GitHub Action 🌎}

cypress · 2021-10-03T20:29:23Z

Test summary

70 • 0 • 0 • 0 • 0

Run details


Project	onearmy-community-platform
Status	Passed
Commit	`ab03241`
Started	Oct 5, 2021 7:44 PM
Ended	Oct 5, 2021 7:50 PM
Duration	06:11 💡
OS	Linux Ubuntu - 20.04
Browser	Multiple

View run in Cypress Dashboard ➡️

This comment has been generated by cypress-bot as a result of this project's GitHub integration settings. You can manage this integration in this project's settings in the Cypress Dashboard

chrismclarke · 2021-10-04T02:46:47Z

src/pages/Howto/Content/Howto/Howto.tsx

+            search:
+              '?search=' +
+              (this.props?.match?.params?.slug).replace(/\-/gi, ' '),
+          }}


Tested this out and really like the functionality, nice idea to also replace the dashes and make it look more human-readable

chrismclarke

Thanks for the contribution, really nice addition to have.
From my side I really like this and think it works well. I'll quickly check in with Dave before merging to see if he has any strong opinions whether we should also display some sort of message (hopefully not, as that could get a tiny bit messier assuming we would need to pass an extra URL param to let the page know it was redirected to after failed search or some other means)

chrismclarke

Just to quickly note, as mentioned on the call would be good to have some sort of message to show to let users know that the howto they were searching for couldn't be found.

Feel free to draft/design any first pass at this that you want and once the PR is updated tag Dave in review for feedback on the design and if you have any concerns about the code feel free to tag me for re-review.

thisislawatts · 2021-10-04T20:07:10Z

@davehakkens see below for a preview of how the search page looks if you arrive via a 404 how to link. I've changed the heading and introduced a sub header as an extra prompt for the user. Let me know what you think:

davehakkens

What a beautiful artwork @thisislawatts!
But yeh, looks neat :)

chrismclarke

One small issue I noticed, is that the source does not reset after navigation, and so the message stays there even after the user opens a howto and returns to a clean howto page (source no longer in URL). So maybe we need to move the check to the list page itself onComponentDidMount/useEffect hook, or just add something to the store to reset when setting an active howto?

When navigating back the URL changes to just be /howto but the message still exists - would be good to revert to standard page message

Precious.Plastic.Community.mp4

thisislawatts · 2021-10-05T05:57:05Z

Thanks @chrismclarke, very helpful! The persistence of state is an existing behaviour which my change makes more visible 🤔

Try the following (desktop device):

Perform a search on https://community.preciousplastic.com/how-to, for example 🔍 table lamp
Navigate to Events using the top menu
Click back to the How to listing, the original search query remains in the box.

Is that deliberate and designed behaviour?

chrismclarke · 2021-10-05T18:21:13Z

Oh, I see what you mean, the search param disappears but the box and search contents persist. No, none of this was active design choices and I don't think has been noticed up till now. I'm guessing probably easiest if we merge this in and create another issue to address?

Unless there was any quick fix you wanted to apply - I'm assuming the expected functionality should be to take the URL bar as main source of truth, so that if navigating back via browser back button search will be retained in URL and UI, but if navigating by menu bar or other triggered methods URL and page will be clean state. But not sure how quick a fix this would be so happy to write-up in another issue if easier to manage that way

thisislawatts · 2021-10-05T18:36:05Z

Yes, I am still gaining context on the MobX module and state management, I found this interesting article entitled, How to decouple state and UI (a.k.a. you don’t need componentWillMount), from 2016.

Interesting, but definitely not straight forward to implement if it's even a valid approach. My experience with SPA of this complexity is limited. So I am here to learn.

thisislawatts · 2021-10-05T18:36:34Z

In the meantime I will take a look at a hotfix

chrismclarke · 2021-10-05T18:44:20Z

Yeah no worries at all. I know the existing codebase is a bit messy around these things (we have lots of different lifecycle hooks used in different ways, some of which are now deprecated (e.g. componentWillMount vs componentDidMount vs UseEffect vs unsafe_componentWillReceiveProps etc.)), and mobx itself has had quite a few breaking changes moving through v4 -> v5 etc.

So yeah, as long as you're having fun basically! In the long term any slow, incremental improvements will also be appreciated, and if you find while working on any of this stuff that there's particular pain points you come across feel free to reach out on slack and I can let you know if I can share any insights around it (always happy to try help speeding up learning curves).

thisislawatts · 2021-10-05T19:06:15Z

Useful context @chrismclarke, I've pushed a fix which uses the UNSAFE_componentWillMount hook for the time being but I think there's an opportunity to a refactor at a later date.

chrismclarke · 2021-10-07T16:53:29Z

Nice - makes sense to me for now. There's still the issue that the search box contents persist across navigation (unrelated to this pr) so can just do the extra bit of refactoring when we turn to address that

thisislawatts · 2022-01-29T14:06:31Z

🎉 This PR is included in version 1.7.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

github-actions bot added the Mod: HowTo 📰 label Oct 3, 2021

thisislawatts commented Oct 3, 2021

View reviewed changes

davehakkens added the Review allow-preview ✅ Has received manual check for malicious code and can be safely built for preview label Oct 3, 2021

chrismclarke mentioned this pull request Oct 4, 2021

[feature request] - Fallback for not found howtos #1227

Closed

chrismclarke self-requested a review October 4, 2021 02:41

chrismclarke reviewed Oct 4, 2021

View reviewed changes

chrismclarke self-requested a review October 4, 2021 02:47

chrismclarke approved these changes Oct 4, 2021

View reviewed changes

feat: adds a helpful heading message for the viewer

68f39e4

chrismclarke self-requested a review October 4, 2021 20:00

chrismclarke requested changes Oct 4, 2021

View reviewed changes

chrismclarke requested a review from davehakkens October 4, 2021 20:28

davehakkens approved these changes Oct 4, 2021

View reviewed changes

chrismclarke self-requested a review October 4, 2021 21:40

chrismclarke requested changes Oct 4, 2021

View reviewed changes

fix: unload state based on URL location

2b3ca93

test: update to align with implementation

ab03241

thisislawatts requested a review from chrismclarke October 6, 2021 18:22

chrismclarke approved these changes Oct 7, 2021

View reviewed changes

chrismclarke merged commit 08c7191 into ONEARMY:master Oct 7, 2021

thisislawatts added the released label Jan 29, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: adds redirect for 404 how tos #1225

feat: adds redirect for 404 how tos #1225

thisislawatts commented Oct 3, 2021 •

edited by chrismclarke

thisislawatts Oct 3, 2021

chrismclarke Oct 4, 2021

github-actions bot commented Oct 3, 2021 •

edited

cypress bot commented Oct 3, 2021 •

edited

chrismclarke Oct 4, 2021

chrismclarke left a comment

chrismclarke left a comment

thisislawatts commented Oct 4, 2021

davehakkens left a comment

chrismclarke left a comment •

edited

thisislawatts commented Oct 5, 2021

chrismclarke commented Oct 5, 2021

thisislawatts commented Oct 5, 2021

thisislawatts commented Oct 5, 2021

chrismclarke commented Oct 5, 2021 •

edited

thisislawatts commented Oct 5, 2021

chrismclarke commented Oct 7, 2021

thisislawatts commented Jan 29, 2022

feat: adds redirect for 404 how tos #1225

feat: adds redirect for 404 how tos #1225

Conversation

thisislawatts commented Oct 3, 2021 • edited by chrismclarke

Description

Git Issues

thisislawatts Oct 3, 2021

Choose a reason for hiding this comment

chrismclarke Oct 4, 2021

Choose a reason for hiding this comment

github-actions bot commented Oct 3, 2021 • edited

cypress bot commented Oct 3, 2021 • edited

Test summary

Run details

chrismclarke Oct 4, 2021

Choose a reason for hiding this comment

chrismclarke left a comment

Choose a reason for hiding this comment

chrismclarke left a comment

Choose a reason for hiding this comment

thisislawatts commented Oct 4, 2021

davehakkens left a comment

Choose a reason for hiding this comment

chrismclarke left a comment • edited

Choose a reason for hiding this comment

thisislawatts commented Oct 5, 2021

chrismclarke commented Oct 5, 2021

thisislawatts commented Oct 5, 2021

thisislawatts commented Oct 5, 2021

chrismclarke commented Oct 5, 2021 • edited

thisislawatts commented Oct 5, 2021

chrismclarke commented Oct 7, 2021

thisislawatts commented Jan 29, 2022

thisislawatts commented Oct 3, 2021 •

edited by chrismclarke

github-actions bot commented Oct 3, 2021 •

edited

cypress bot commented Oct 3, 2021 •

edited

chrismclarke left a comment •

edited

chrismclarke commented Oct 5, 2021 •

edited