Skip to content

Conversation

@HeVictor
Copy link
Contributor

@HeVictor HeVictor commented Jan 3, 2023

Have noticed that the AdventureSite tutorial page has been broken for quite a while from the Adventure Site link on the main Terasology GitHub page, so decided to give a go at fixing it in a way that works with the latest react-router-dom library version. The PR also includes a couple of small typo fixes.

The proposed solution here is just an initial attempt to make the tutorial journey render properly again so I'm open to suggestions that enhances the implementation in any way. I have tested the fix using yarn start locally and most of the routes appear to be working but by no means have I tested everything so if there's a list of styling/functionality-related things to verify I'm happy to go through that and check that it's all working as intended and reachable through the main Terasology website.

const [userName, setuserName] = useState('Test-User >');

const [searchParams, setSearchParams] = useSearchParams();
const pathId = searchParams.get('pathId');
Copy link
Contributor Author

Choose a reason for hiding this comment

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

From what I can tell in the previous implementation I'm guessing pathId is used as a search parameter. In react-router-dom v6 the new way to get search parameters is useSearchParams so I updated the code to use this to fetch the actual 'pathId' value

}
/>
</>
)
Copy link
Contributor Author

@HeVictor HeVictor Jan 3, 2023

Choose a reason for hiding this comment

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

The original implementation was giving an error that complained <Route> component was not being wrapped by a <Routes> component. While adding another <Routes> component on top does solve the issue like the error message said, I've decided to change the implementation to what's recommended by the react-router-dom v6 docs that utilises the createRoutesFromElements function which also converts this app to use a BrowserRouter instead of a HashRouter

<>
<Route path="/map" element={PathMap} />
<Route
path="/AdventureSite"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I might need some clarification here, from yarn start I see that the URL that the app launches on is always http://localhost:3000/AdventureSite which is why I needed to specify the path to be /AdventureSite here so that the app renders the tutorial journey properly on startup. I'm unsure if this will work as-is with how the site is rendered on the Terasology website itself, especially as the link there uses a hash in the URL but the implementation here has been updated to use a BrowserRouter


export default function createPathIdQuery(paramValue) {
return PATH_ID_QUERY_PARAM + paramValue;
} No newline at end of file
Copy link
Contributor Author

Choose a reason for hiding this comment

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

As I wanted to avoid making huge underlying changes I've made the existing Link components from the different tutorial journey options update the pathId query parameters by simply appending the end of the URL with ?pathId={paramValue} as shown by the util function here. There is a way to use the setSearchParams function from the v6 'react-router-dom' but that might require more changes to use something other than Link components so I've done it this way for now

},
{
name: 'Done. What else should I know about?',
jump: 'u0u2u0u0',
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This particular component was causing a circular dependency problem that crashed the app on startup as it referred to the ContributionResources which in turn referred back to this Git component. I've fixed this by replacing the 'child' param with the equivalent 'jump' param 'pathId' value that points to the same expected page

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.

1 participant