Catalog cards #63

max172-hqt · 2025-01-21T15:11:51Z

Description

Please include a summary of the changes and relevant context. List any dependencies that are required for this change.

Related issue: #62

Type of change

Delete options that are not relevant.

New feature (non-breaking change which adds functionality)

Instructions

Describe the steps required to verify that the changes are functioning as expected.

How Has This Been Tested?

Describe the tests that you ran to verify the changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration.

Storybook for multiple use cases of CatalogCard and CatalogList
Component tests

Checklist:

I have read the CONTRIBUTING doc
I have performed a self-review of my own code
I have commented my code, particularly in hard-to-understand areas
I have updated Storybook with relevant examples
I have made corresponding changes to the documentation
I have added tests that prove my fix is effective or that the feature works
New and existing unit tests pass locally with my changes

JBurkinshaw

A few comments in the code.

General questions:

Should we be adding loading states for async data?
With MAX_LENGTH, does this need to be flexible? i.e. should we be passing it in as an optional property while keeping the default?
Will the cards require click handlers in the future?

JBurkinshaw · 2025-01-21T19:39:13Z

src/components/core/CatalogCard.tsx

+export type IndicatorTag = "API" | "Catalog";
+
+export interface BaseCatalog {
+    // eslint-disable-next-line react/no-unused-prop-types


Can you think of a way to use id in here?
If not, would it make sense to make it optional?

This is used in the Catalog component when rendering each Catalog card (set key), so we still expect the data to have id property. It's showing a warning because the file we're in doesn't use id. For the interfaces that are used in multiple places, should we move it to somewhere else?

I would love to do something like this since CatalogList (rename later) and CatalogCard are so closely related, but not sitting well with our structure.

components

Catalog (directory)

CatalogList.tsx

CatalogCard.tsx

... others tsx

1 file for typescript types if needed

css module

test

index.ts to export reusable components

I think that structure makes sense here and allows the interface to be in a different file too. The current flat directory structure is no very scaleable so moving to something more like this makes more sense to me.

I have done this change in my local. Should I push it now or should we discuss more about it, as it's gonna change the way we structure drastically? If doing this, we won't differentiate core / composite components anymore in the codebase, but still have component types on storybook.

JBurkinshaw · 2025-01-21T19:45:43Z

src/components/core/CatalogCard.tsx

+import type { DateValue } from "react-aria";
+import styles from "./CatalogCard.module.css";
+
+export type TemporalExtent = [DateValue, DateValue?];


Could we be more specific about DateValue null/undefined cases?
Maybe not, but I'm interested to know what happens if the 1st DateValue in the array is nullish.

Here we expect the library user to pass in one or two DateValue for TemporalExtent

So if user passes in null for the first item or anything that doesn't satisfy the type, the editor is gonna show an error. I think type checking with typescript is good enough. Wdyt?

Ok, I am happy with this approach in that case

I've been putting some more thought into this as I modify the DatePicker component.
I'm not convinced that passing in props which use react-aria types like DateValue is ideal. It introduces another dependency and requires the developer to understand the types in the library.
There are a few alternatives, but I'm leaning to passing in regular JS Date objects and converting them to a react-aria type like DateValue within the component. What do you think?

As an example, here's what I'm working on for the DatePicker: #66

Sounds good to me

JBurkinshaw · 2025-01-21T19:48:14Z

src/components/core/CatalogCard.tsx

+            shouldTruncateDescription
+                ? `${initialDescription.slice(0, MAX_LENGTH)}${isLongText ? "..." : ""}`
+                : initialDescription,


Consider moving this to a utility function. Something like this:

const truncateDescription = (text: string, maxLength: number) => { if (text.length <= maxLength) return text; return `${text.slice(0, maxLength)}...`; };

There's actually a utils directory. The aim is to abstract useful utilities so we can use them without components in the future if required.

JBurkinshaw · 2025-01-21T19:49:21Z

src/components/core/CatalogCard.tsx

+                    onPress={() =>
+                        setShouldTruncateDescription(!shouldTruncateDescription)
+                    }
+                    aria-expanded={!shouldTruncateDescription}


Also add an aria-label property here for accessibility

alanjleonard

Re Catalog count:

I think we'll need something in place if the catalog count is in the hundreds+ Like if there's 3 digits in there.
Also 8px is too small. I would bump it up to at least 12px. @e-e-james thoughts on font size?

alanjleonard · 2025-01-21T20:52:56Z

Re: General questions @JBurkinshaw

Huy mentioned that the loading state would make more sense on the list of cards, as opposed to an individual card.
My fear with making the MAX_LENGTH optional is that it would break the card component if there was a really long description. I think we keep it in.
I'm not sure about a click handler right now for this component. If anything, cards in general could have a default action when clicked then secondary actions would be in an overflow menu. In this case it would be Browse. Let's leave it as is for now and we can revisit it if it comes up again for a more generic card component.

max172-hqt · 2025-01-21T21:04:34Z

I also think an empty state should be implemented.
We can have a prop maxDescriptionLength with default value of MAX_LENGTH, and let the user set another max length if needed.

@alanjleonard I'm working on this. We can remove fixed width and height, add some padding and aspect-ratio 1/1 so that the circle will grow as we have more digits. 4 digits or less look fine.

.catalogHeader span {
    ...
    padding: 4px;
    border-radius: 50%;
    aspect-ratio: 1/1;
    flex-shrink: 0; // not sure if this is needed, just to make sure it is not shrunk in a flex layout
    min-width: 16px;
}

JBurkinshaw · 2025-01-21T23:13:51Z

I also think an empty state should be implemented.

We can have a prop maxDescriptionLength with default value of MAX_LENGTH, and let the user set another max length if needed.

@alanjleonard I'm working on this. We can remove fixed width and height, add some padding and aspect-ratio 1/1 so that the circle will grow as we have more digits. 4 digits or less look fine.
.catalogHeader span {
    ...
    padding: 4px;
    border-radius: 50%;
    aspect-ratio: 1/1;
    flex-shrink: 0; // not sure if this is needed, just to make sure it is not shrunk in a flex layout
    min-width: 16px;
}

@max172-hqt maxDescriptionLength with a default set makes sense to me. It would be optional in the sense that if no value is passed, it will default to a sensible length e.g. 250

JBurkinshaw · 2025-01-22T19:52:21Z

src/components/composite/Catalog.module.css

+    border-radius: 50%;
+    display: inline-flex;
+    justify-content: center;
+    align-items: center;
+    color: #fff;
+    font-size: 10px;


@max172-hqt Another thought on this. Can you use CSS variables from src/components/styles/theme.css to set as many of these as possible please? Thinking in terms of color, font size, border radius and whatever else you can apply. ~~In the work I'm doing on the datepicker I have added a new variable --border-radius: 4px; but we'll need some input from the design team to align on the approach we take here~~ Edit: Forget this. Let's stay with the existing border-radius-* CSS vars

On this, I feel like we’re trying to figure out some default values / spacing in the theme.css file, which are very similar to defaults from TailwindCSS or similar libraries. A few things that made it work really well that I think we can adopt are:

consistent spacing. Everything is proportional to 1rem, which is the default browser / html font size (16px). So when user changes default font size, everything will scale correctly. Right now we override default font size to 14px, so it doesn’t make sense to use rem. Reference

have a normalize css rules. This is to solve browser inconsistencies and make working with semantic HTML tags easier (eg. remove margins from h1,h2…p tags). Looks like we’re mostly using div tags at the moment.

other than that, a lot of default colors, utility classes, easy to work with dark themes and responsive UIs

For context, currently we're using the Vanilla CSS "starter kit" found here. It comes with css files for the base react-aria components, along with a theme.css file which we based our theme.css file in spk-components on.

I am fine with using consistent spacing proportional to 1rem if we're all onboard.

Normalize css is something I've thought about and I think it's a good idea. I think adding it should be tackled as a separate issue though.

I think the default colours are deliberately minimal in our theme.css. It should handle dark/light themes too but we haven't focussed on that yet

alanjleonard · 2025-02-20T16:38:11Z

I think to keep inline with how we're going to render the description in the different detail sections, we can

Render the markdown. If the rendered markdown is longer than 300 characters. Add an x-overflow to the container.

Probably makes sense to do this in a different ticket, or after that card is complete. I'll make another placeholder ticket for it.

max172-hqt added 6 commits January 20, 2025 21:25

feat: Implement individual catalog card

82710ca

Add render prop

66774c3

feat: Implement Catalog list container

4a683af

feat: Add id to catalog object

7f2343c

fix: Move story to correct directory

1e6cd1c

fix: Fix catalog number style

379d5ec

JBurkinshaw reviewed Jan 21, 2025

View reviewed changes

alanjleonard requested changes Jan 21, 2025

View reviewed changes

max172-hqt added 7 commits January 22, 2025 10:09

feat: Add truncate text util function

1ce92c5

fix: Add aria-label to read more button

fb6fb3f

fix: Use default Date object

a0014b9

fix: Convert date to utc string

841eef4

Add empty screen

53df28e

chore: Rename to CatalogList

071ca25

Add test for catalog card

3f43f0f

JBurkinshaw reviewed Jan 22, 2025

View reviewed changes

max172-hqt added 2 commits January 22, 2025 15:23

feat: Add test for CatalogList

1936678

fix: Create Date object with UTC

b8e0e2d

max172-hqt marked this pull request as ready for review January 22, 2025 20:37

max172-hqt added 3 commits January 23, 2025 19:46

fix: Use ID as title when title is not provided

63aa0fa

chore: Add test stories

2df5b84

chore: Update directory structure for Catalog

a99a3f5

JBurkinshaw approved these changes Jan 24, 2025

View reviewed changes

fix: Add background white to card

5921457

alanjleonard mentioned this pull request Feb 20, 2025

Render Markdown for catalog card description. #77

Open

JBurkinshaw mentioned this pull request Feb 26, 2025

[FEATURE] Details Pane - Catalog #65

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Catalog cards #63

Catalog cards #63

max172-hqt commented Jan 21, 2025 •

edited

Loading

JBurkinshaw left a comment

JBurkinshaw Jan 21, 2025

max172-hqt Jan 21, 2025

JBurkinshaw Jan 21, 2025

max172-hqt Jan 22, 2025

JBurkinshaw Jan 21, 2025

max172-hqt Jan 21, 2025

JBurkinshaw Jan 21, 2025 •

edited

Loading

JBurkinshaw Jan 22, 2025 •

edited

Loading

max172-hqt Jan 22, 2025

JBurkinshaw Jan 21, 2025

JBurkinshaw Jan 21, 2025

alanjleonard left a comment

alanjleonard commented Jan 21, 2025

max172-hqt commented Jan 21, 2025

JBurkinshaw commented Jan 21, 2025

JBurkinshaw Jan 22, 2025 •

edited

Loading

max172-hqt Jan 22, 2025

JBurkinshaw Jan 23, 2025

alanjleonard commented Feb 20, 2025

Catalog cards #63

Are you sure you want to change the base?

Catalog cards #63

Conversation

max172-hqt commented Jan 21, 2025 • edited Loading

Description

Type of change

Instructions

How Has This Been Tested?

Checklist:

JBurkinshaw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JBurkinshaw Jan 21, 2025 • edited Loading

Choose a reason for hiding this comment

JBurkinshaw Jan 22, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alanjleonard left a comment

Choose a reason for hiding this comment

alanjleonard commented Jan 21, 2025

max172-hqt commented Jan 21, 2025

JBurkinshaw commented Jan 21, 2025

JBurkinshaw Jan 22, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alanjleonard commented Feb 20, 2025

max172-hqt commented Jan 21, 2025 •

edited

Loading

JBurkinshaw Jan 21, 2025 •

edited

Loading

JBurkinshaw Jan 22, 2025 •

edited

Loading

JBurkinshaw Jan 22, 2025 •

edited

Loading