Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Visual update and fix for scenario and scene selection #259

Merged
merged 10 commits into from
Oct 20, 2024

Conversation

wjin-lee
Copy link
Member

Overview

This ticklet is an amalgamation of what should have been three separate PRs. Given the time constraints before demo, I am opting to do one PR to avoid merge conflicts.

  • The first section describes the proposed visual update to the home page.
  • The second section describes the fix to SceneSelectionPage.
  • The third section describes the fix to a backend crash that would occur with malformed scenario IDs.

Proposed Visual Updates

Originally, this was going to be a simple home page touch up. However, due to the highly coupled nature of ListContainer which displays the user-created scenarios, assigned scenarios and scenes, a larger refactor was required.

See below for the old design of the home page:
image

There were several improvements identified:

  • Side bar layout could be improved. Everything is squished up top, leaving big spaces near the bottom.
  • Side bar button layouts also seem odd, with two different "groups" of actions being identified: 1) scenario-related (create, edit, play, delete) and 2) app buttons (logout, help)
  • Created and Assigned scenarios has default font and styling. (looks unpolished)
  • the scenario name inputs seem unfinished due to its sharp corners and pure white background (as to the softer blue-white background)
  • the scenario/scene thumbnails change in width, creating a grey underflow (seen on the right).

There are also underlying code structure issues:

  • Headings and titles, alongside some spacing logic is included within ListContainer effectively coupling ListContainer and outer components like ScenarioSelectionPage and SceneSelectionPage
  • Created and Assigned scenarios is coupled within one component: ListContainer with duplicated code (literally copy+paste with minor modifications).
  • To make this worse, ListContainer is littered with conditional elements to also work as the scene selection page alongside the scenario selection page.

The new design aims to address this with a major structural refactor.
image
image

  • Sidebar now is split into three distinct sections. 1) uoa logo, 2) scenario-related actions 3) app-related actions.

  • Sidebar buttons have now had icons added.

  • ListContainer has now been refactored into ThumbnailList which purely displays a list of thumbnails. The outer component pages are responsible for the heading and positioning of this list.

  • MonaSans has been applied to the titles, alongside an icon for good measure


Fixes to SceneSelectionPage

When directly trying to access the scene's authoring tool, (e.g. via copy and pasting a URL with the scenario included like http://localhost:5173/scenario/66ff60318aa1f312a9a9cf58), the application would incorrectly return a 401 code, even when logged in.

The expected behaviour here is that the authoring tool correctly loads.

The issue is due to insufficient consideration of our authentication loop, where the requests to fetch the scenario (by the ScenarioContext) are sent before the auth loop updates the user object to the logged-in user. This in turn results in a 401 as we do not have the Bearer token at the time of the requests.

The solution fixes the first oversight by adding a skipRequest flag to the useGet hook to conditionally skip the request (like when we do not have a user object) and simply re-requesting when we obtain the user object.

However, because we don't actually load the "selected scenario" anywhere other than the main page (even though it is in our URL!), we must also fix the scene selection page to retrieve the scenario specified in the URL if the currently selected scenario is missing.


Fixes to the backend scenario endpoints

During testing of the above, another bug was exposed where the backend server would crash if given a malformed scenario ID. It implicitly trusted that all scenario ids requested would be well-formed, which is a poor assumption.

We introduce another middleware that checks for scenario ID validity, and rejecting any that is malformed before it gets to the data access objects. (Note that we check for validity, not existence!)

Copy link
Contributor

@harbassan harbassan left a comment

Choose a reason for hiding this comment

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

looking really good

Copy link
Contributor

@itsatulbox itsatulbox left a comment

Choose a reason for hiding this comment

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

lgtm!

@wjin-lee wjin-lee merged commit df84dd2 into master Oct 20, 2024
5 checks passed
@wjin-lee wjin-lee deleted the wlee_update-home-styling branch October 20, 2024 05:15
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.

3 participants