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

Add /all endpoint documentation #45

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

Add /all endpoint documentation #45

wants to merge 2 commits into from

Conversation

gestchild
Copy link

What does this change?

For #43

N.B. The content.yaml is getting a bit unwieldy. I did attempt to break the file up and make bits reusable, but I ran into a bug with swagger where it only seems to recognise the first external file used in a $ref. Rather than waste any more time on it I ended up just editing the content.yaml file, but we may want to revisit this again in the future.

How to test

You can run this locally with yarn start, and look at the new docs

How can we measure success?

It's up to date and useful for anyone we might onboard.

Have we considered potential risks?

N/A

@gestchild gestchild requested review from a team as code owners December 16, 2024 11:49
Comment on lines +328 to +331
tags:
- All
summary: /all
description: Returns a paginated list of our non-catalogue content, i.e. (articles, books, events, exhibitions, exhibition texts, exhibition highlight tours (BSL), exhibition highlight tours (audio), pages, projects, seasons, and visual stories)
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we/should we reference Addressables? I guess people using the API don't really care about that as it's pipeline side...

Copy link
Contributor

@rcantin-w rcantin-w left a comment

Choose a reason for hiding this comment

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

Screenshot 2024-12-17 at 13 33 38

This is very cool

Comment on lines +804 to +808
required:
- type
- uid
- title
- description
Copy link
Contributor

Choose a reason for hiding this comment

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

Looking at the type here, uid and description are optional, id should be required (I think?), same for all content types

Copy link
Author

Choose a reason for hiding this comment

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

Shouldn't uid be required though? It's a required field in Prismic and we will rely on it for constructing links

Copy link
Contributor

Choose a reason for hiding this comment

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

I can't remember 100% why we made it optional, but we did see instances of it missing it, although it shouldn't really happen anymore. Should we change the type then?

Copy link
Author

Choose a reason for hiding this comment

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

I think so, I'll do that

- uid
- title
- description
- times
Copy link
Contributor

Choose a reason for hiding this comment

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

Times is optional in the content api types

properties:
type:
type: string
description:
Copy link
Contributor

Choose a reason for hiding this comment

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

Is description required if empty?

Copy link
Contributor

Choose a reason for hiding this comment

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

Also I know we haven't done that in the other bits of docs, but should we say the type is "Season" (as it's hard coded?)

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