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

feat: self assessment changes with time and view recommendation button #667

Conversation

katie-gardner
Copy link
Contributor

Description

This PR extends the existing work to the self-assessment page to display the view recommendation button next to the category.

Removes the new recommendation banner, as this is now indicated by the "New" tag next to view recommendation, after a user completes a topic

Notes

  • It wasn't possible to achieve the exact layout on the screenshots in the ticket without hardcoding width overrides in the CSS, and overriding the design system table styling, neither of which is ideal
  • So following UX chat the New tag is now going to go on the left of the button, which only required a small tweak to vertical alignment

@katie-gardner katie-gardner changed the title Feat: Recommendations front end feat: self assessment changes with time and view recommendation button Jun 13, 2024
@jimwashbrook jimwashbrook self-requested a review June 13, 2024 18:11
Copy link
Contributor

@jimwashbrook jimwashbrook left a comment

Choose a reason for hiding this comment

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

It wasn't possible to achieve the exact layout on the screenshots in the ticket without hardcoding width overrides in the CSS, and overriding the design system table styling, neither of which is ideal

I don't think this is necessarily true, or, rather, I think you've been led down the wrong rabbit hole.

There's a few things here, but I think we should discuss tomorrow:

  1. When our task list was originally implemented it was a pattern not a component. So our CSS is... well I have no idea where it comes from. It is now a component though.
  2. It's very common for services to change how the GDS components work/look to a degree
  3. Why is this even a table? It really shouldn't be. I guess it matters less when you've only got 2 columns, but we're changing the component/pattern now right? In which case maybe it is time to rethink how its layed out. Why can't we just switch to flex?

@jimwashbrook
Copy link
Contributor

jimwashbrook commented Jun 14, 2024

I have spent a lot of time on this PR.

You've done a good job, there's a few semi-minor things (e.g. tag colours). I can't test it properly though due to the current issue until the PR goes in.

However, I have a lot of strong feelings on this redesign. None of them good.

To add some thoughts on this (none of which are about your code)

  1. The HTML tags, classes, etc. we're using for this are all wrong. See GDS docs
  2. GDS says we should have a link to the sub-topic for the full-width of each task row. (And this is gonna be awful with multiple columns)
  3. I can't find a single instance of anyone using the task list component (or pattern) with more than 2 columns.
  4. Complete status tag should be white? That's news for me

Even if we decided that we really, really want... this. We're going to have to do some much bigger changes to the GDS task list component than you already have.

image
Not vertically centered

image
Inconsistent height due to error message now going over to two lines

image

3 different sizes. Which was less of an issue when they were right aligned, and only different if they were different statuses. Now we have 2 different sizes for the same status

And this will only get worse as the dates are more varied:
image
🤢

image 🤷

On another note: I can't see Tony having done a re-done design for you? And the design that was provided is incorrect (unless we're going all grey tags? Which... don't get me started).

IMO the way the task list is being changed now violates the intention behind it.

To quote GDS:

"The task list component displays all the tasks a user needs to do, and allows users to easily identify which ones are done and which they still need to do."

"easily identify which ones are done" - nope not with tags all being same colour
"which they still need to do" - given that a user can redo them infinite times; I don't think this ever actually applies

"Users can only move on from the task list when all tasks are shown as ‘Completed’." Well there's no where to move on to.

"Each task within a task list includes a task name and a status. It can also include hint text if you decide this is needed." Well now we're including more, still no help text though.

Anyway that's a lot of words about something that really is nothing to do with this PR it's just highlighted it all for me and I needed somewhere to write my thoughts out ready for tomorrow I guess. Thanks for reading!

@jimwashbrook
Copy link
Contributor

image image image

@katie-gardner
Copy link
Contributor Author

katie-gardner commented Jun 14, 2024

Thanks for all these thoughts

Adding this as one to include in convo - I totally agree that this would be a misuse of task-list, which is why I've changed it to be using the summary component instead, which does meet the multi column design, and since tags are a thing on their own I think we're ok to use those within. But it is essentially tasks so perhaps summary isn't the perfect one either. With summary component I haven't had to override too much.

I wanted to avoid hardcoded widths and heights within but I'm not sure how many options there'll be for the heights and tag widths... you raise good points, probably one for a conversation. The redesign is on the ticket - I did double check the grey tags but it is what the design wants

@katie-gardner katie-gardner merged commit cdb1c16 into feat/self-assessment-layout Jun 14, 2024
@katie-gardner katie-gardner deleted the feat/211792/recommendations-front-end branch June 14, 2024 11:47
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.

2 participants