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

[DO NOT MERGE] Update search dashboard #165

Open
wants to merge 14 commits into
base: main
Choose a base branch
from

Conversation

jon-kirwan
Copy link
Contributor

@jon-kirwan jon-kirwan commented Jan 13, 2025

What

Amend 'Live Searches' dashboard:

  • Amend styles (larger copy, more spacing, black background and flex layout)
  • Add 'users on GOV.UK in the last 24 hours' column
  • Add footer logo
  • Remove 'popular' and 'recently published' columns

Why

To be displayed at The White Chapel Building.

- Amend styles (larger copy, more spacing, black background and flex layout)
- Add 'users on GOV.UK in the last 24 hours' column
- Add footer logo
- Remove 'popular' and 'recently published' columns
@govuk-ci govuk-ci temporarily deployed to govuk-display-screen-jan-2025 January 13, 2025 14:38 Inactive
@govuk-ci govuk-ci temporarily deployed to govuk-display-screen-jan-2025 January 14, 2025 12:22 Inactive
@govuk-ci govuk-ci temporarily deployed to govuk-display-screen-jan-2025 January 14, 2025 12:24 Inactive
Copy link
Contributor

@JamesCGDS JamesCGDS left a comment

Choose a reason for hiding this comment

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

Nice work! Similar to the other dashboard PR I haven't delved too deeply into the code - I've primarily checked that everything looks okay (which it indeed does!). Although it looks like we may have a similar issue (not sure if it's an issue though tbh) of blank space beneath the live searches on the portrait 48-inch screen - I wonder if there'd be any mileage in increasing the size of the container a bit? Maybe 25% so we take up a bit more space but not so much that it becomes overwhelming? What do you think?

I've also left a couple of comments/questions but nothing major 👍

public/index.html Outdated Show resolved Hide resolved
public/index.html Show resolved Hide resolved
trafficYesterday.$el = $('#traffic-count-yesterday');

trafficYesterday.reload();
// window.setInterval(trafficYesterday.reload, 30e3);
Copy link
Contributor

Choose a reason for hiding this comment

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

Nitpick: potential rogue comment here? I may be getting too nitpicky here though, especially if this is prototypal code, so feel free to ignore this one

Copy link
Contributor Author

@jon-kirwan jon-kirwan Jan 16, 2025

Choose a reason for hiding this comment

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

This code updates the "users in the last 30 minutes" value every 30 minutes. However, since the value represents users from the last day, not 30 minutes, this would need to be changed so I've modified it to run once per day using the following code:

window.setInterval(myObj.reload, 86400000);

This change triggers the update every 24 hours. Whether this change is necessary probably depends on whether the dashboards will be displayed on subsequent days after a visit. If they are, we'll want the data to stay up-to-date.

@jon-kirwan
Copy link
Contributor Author

Nice work! Similar to the other dashboard PR I haven't delved too deeply into the code - I've primarily checked that everything looks okay (which it indeed does!). Although it looks like we may have a similar issue (not sure if it's an issue though tbh) of blank space beneath the live searches on the portrait 48-inch screen - I wonder if there'd be any mileage in increasing the size of the container a bit? Maybe 25% so we take up a bit more space but not so much that it becomes overwhelming? What do you think?

I've also left a couple of comments/questions but nothing major 👍

Yep, that looks a bit odd on the larger screen. I've modified the number of results from 20 to 48 (which seems to fill the screen).

@jon-kirwan jon-kirwan marked this pull request as ready for review January 16, 2025 10:03
Copy link
Contributor

@JamesCGDS JamesCGDS left a comment

Choose a reason for hiding this comment

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

This looks great, awesome work 🎉 Just spotted one thing - I think the latest commit has introduced a UI quirk where the logo "bounces":

live-search-2.mov

Things seemed to be looking pretty good for me in the penultimate commit though:

live-search-1.mov

Would it be possible to reinstate the styles/behaviour seen in the video immediately above?

Also got a couple of other questions:

  • Are we thinking of keeping this code on its own branch and deploying the branch to Heroku in order to preserve main? In which case I believe we wouldn't want to merge this?
  • If we do want to merge this then I guess one concern might be needing to prevent anything else from being merged into main so nothing changes our dashboard. Having said that, this repository is hardly ever changed (prior to Guilhem's revival of this repository on 16th December, the last commit was in July and that was to archive it) so perhaps we don't need to be too worried

@jon-kirwan jon-kirwan changed the title Update search dashboard [DO NOT MERGE] Update search dashboard Jan 16, 2025
@jon-kirwan
Copy link
Contributor Author

Would it be possible to reinstate the styles/behaviour seen in the video immediately above?

Yep, that should be fixed now. I set the main container to hide overflow and reduced the number of items displayed by one which seems to work.

@govuk-ci govuk-ci temporarily deployed to govuk-display-screen-jan-2025 January 17, 2025 11:28 Inactive
- Hide content after nth-child specified number
- Add text-wrap-balance to stop orphan text wrapping onto new line
@govuk-ci govuk-ci temporarily deployed to govuk-display-screen-jan-2025 January 20, 2025 14:20 Inactive
@govuk-ci govuk-ci temporarily deployed to govuk-display-screen-jan-2025 January 20, 2025 16:00 Inactive
@govuk-ci govuk-ci temporarily deployed to govuk-display-screen-jan-2025 January 20, 2025 16:36 Inactive
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