Skip to content

Pascal/pagination numbers fire #891

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

Merged
merged 9 commits into from
Jun 2, 2025
Merged

Conversation

Pascal-Delange
Copy link
Contributor

@Pascal-Delange Pascal-Delange commented May 20, 2025

Content

Part of preparatory work for checkmarble/marble-backend#1010.
It's a minimal downgrade of user experience, but will make things SO MUCH simpler on the backend side.

Sneak peak

Screenshots updated, see comment below

What pagination without the rank numbers could look like:
first page
Capture d’écran 2025-05-20 à 22 33 52
later page
Capture d’écran 2025-05-20 à 22 33 56
if first and last element have created_at in the same month
Capture d’écran 2025-05-20 à 22 34 03
NB: to discuss in which order to write it - on those screenshots it's in the order "from [first row] to [last row]", but that's opposite to temporal order with this pagination order.

While in the case manager it would be
Capture d’écran 2025-05-20 à 22 40 47
because the order is not based only on the case dates (also on "boosted" status).

Disclaimer

As it is, it displays french format dates even though my selected locale is "english". This is not directly related to this PR, but is a side effect of a partial handling of language/format state in the app/browser, as explained in packages/app-builder/src/utils/format.ts.

@Pascal-Delange
Copy link
Contributor Author

Apparently this passed the "early slack design review", so pushing this for code review now.

@Pascal-Delange Pascal-Delange marked this pull request as ready for review May 21, 2025 07:28
@Pascal-Delange Pascal-Delange requested review from ChibiBlasphem and a team May 21, 2025 07:29
@@ -270,42 +271,6 @@ function SearchById() {
);
}

// Temporary disabled
Copy link
Contributor

Choose a reason for hiding this comment

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

Temporary since November, 2023. 😆

Copy link
Contributor Author

Choose a reason for hiding this comment

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

no such thing as temporarily commented code :D

@apognu
Copy link
Contributor

apognu commented May 21, 2025

We might want to also display the seconds (or change the formatting some way) if the first and last elements are within the same minute.

It is quite likely that an organizaiton will have more than 25 (or even more than 50) decisions made within the same minutes, which would mean several pages would get the same pagination formatting, which in my opinion WouldBeBad.

[...] 23:48 to 23:48

@Pascal-Delange
Copy link
Contributor Author

You're right, will add tonight (or whenever I finish the PR)

@Pascal-Delange
Copy link
Contributor Author

Pascal-Delange commented May 23, 2025

  • different day
Capture d’écran 2025-05-23 à 17 34 27
  • same day, different minute: you imagine it

  • same day, different second

Capture d’écran 2025-05-23 à 17 34 33
  • same second
Capture d’écran 2025-05-23 à 17 34 49

Ok, new version where we show more precise time if it's in the same minute is ready. If it's in the exact same second, even more compact display (and it's more likely to change from page to page)

@Pascal-Delange Pascal-Delange force-pushed the pascal/pagination-numbers-fire branch from 7c177a6 to eb2a2e4 Compare May 23, 2025 15:40
components={{ StartToEnd: <span style={{ fontWeight: 'bold' }} /> }}
values={{ start, end }}
/>
{hideBoundaries ? null : endFormatted === null ? (
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 actually hate nested ternaries, but js somehow pushes me to do it

@Pascal-Delange
Copy link
Contributor Author

Pascal-Delange commented May 23, 2025

I'm getting this error however
Capture d’écran 2025-05-23 à 17 46 19

Fixed in the next commit ?

@Pascal-Delange Pascal-Delange requested a review from siiick May 23, 2025 15:50
@siiick
Copy link
Contributor

siiick commented May 23, 2025

I pushed commit 0e5a3035 with the following changes:

  • Refactored the getStartAndEndFormatted function into a FormattedDatesRange component that handles both formatting and rendering of date ranges.
  • Introduced distinct wordings for: Same second / Same day / Different days
    This allows us to render ranges like:

From 23 Apr 2025, 10:52 to 28 Apr 2025, 10:53
On 28 Apr 2025 from 10:52 to 10:53
On 28 Apr 2025 at 10:53

  • Simplified the root component and clarified separation of concerns.

@Pascal-Delange Pascal-Delange force-pushed the pascal/pagination-numbers-fire branch from 0e5a303 to c3dd77c Compare May 28, 2025 21:16
@Pascal-Delange
Copy link
Contributor Author

Last iteration on design.

decisions, standard case
Capture d’écran 2025-06-01 à 21 31 09

decisions, all in same second case
Capture d’écran 2025-06-01 à 21 31 21

no cases matches filter
Capture d’écran 2025-06-01 à 21 31 28

cases, standard case
Capture d’écran 2025-06-01 à 21 31 35

cases, last page
Capture d’écran 2025-06-01 à 21 31 40

@Pascal-Delange
Copy link
Contributor Author

The last commit is a refactoring proposition, read it separately.
There was a formatDateTime util function, that included default presets for datetime display.
In the end, half of the places where it was used were passing some of the default props as undefined to suppress them, also the default options (timeformat: "short") are incompatible to mix with numeric formats ("year": "2-digit"...), so I figured it's really more simple and explicit to just let whoever is using the function, choose what format is reasonable for them.

@Pascal-Delange Pascal-Delange force-pushed the pascal/pagination-numbers-fire branch from aef406c to 3726931 Compare June 1, 2025 20:00
Copy link
Contributor

@siiick siiick left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@Pascal-Delange Pascal-Delange merged commit 6bd3c5d into main Jun 2, 2025
4 checks passed
@Pascal-Delange Pascal-Delange deleted the pascal/pagination-numbers-fire branch June 2, 2025 15:18
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