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

Use Schema to make frontend generic (part 1) #154

Merged
merged 24 commits into from
Dec 13, 2024
Merged

Conversation

psrpinto
Copy link
Member

@psrpinto psrpinto commented Dec 12, 2024

This is a partial fix for #137

Everything should still function, this is just refactoring.

This PR makes it so that the frontend uses the schema to determine what fields a subject has. It's mostly all cherry-picked from #132 with slight changes.

The frontend code has been (almost, see Next Steps below) fully adapted to use the schemas, which means that the code is now generic. All the specific types (BlogPost, Page, BlogPostBlueprint, etc) have been removed, and the only types left are generic (Subject, Blueprint).

This results in the code being massively simplified, and adding a new subject type does no longer require changing a bunch of files, and copy-pasting code around.

A new SubjectsApi has also been introduced, and the specific BlogPostsApi and PagesApi were removed. However, internally, SubjectsApi, still differentiates between subject types because it needs to call different endpoints depending on the subject type.

Next steps

See sub-issues in #137

@psrpinto psrpinto marked this pull request as ready for review December 12, 2024 13:48
@psrpinto psrpinto requested review from ashfame and akirk December 12, 2024 13:50
@akirk
Copy link
Member

akirk commented Dec 12, 2024

Nice work with the simplification! Could you comment on what is still required in the JSON file at the moment and what we need?

Judging from the "Import Posts" button, one thing that we need is a plural term in the schema (see get_post_type_labels()),

I also tried to rename the title field resulting in the error "Unexpected Application Error! field is undefined"

@psrpinto
Copy link
Member Author

psrpinto commented Dec 12, 2024

Great point about the plurals @akirk, opened #158 to tackle that. Also, great catch (thanks!) with the error that shows after renaming the title, opened #159 for that one.

Could you comment on what is still required in the JSON file at the moment and what we need?

I'm not sure I understand what you mean by this. Do you mean whether there are still properties missing in the schema that we would need, and/or whether there are properties that are already in the schema that we are not leveraging? (I opened #157 which is related to this)

I created tickets for all things mentioned here and others I could think of, linked them as sub-tickets of #137, and also added them to the milestone.

@psrpinto psrpinto changed the title Use Schema to make frontend generic Use Schema to make frontend generic (part 1) Dec 12, 2024
@psrpinto
Copy link
Member Author

psrpinto commented Dec 13, 2024

@akirk @ashfame Can I merge this? There's already #160 which depends on this one, and I'd like to continue working in other PRs but don't want to have a long stack of PRs.

Copy link
Member

@akirk akirk left a comment

Choose a reason for hiding this comment

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

Ready for the next iteration

Copy link
Member

@ashfame ashfame left a comment

Choose a reason for hiding this comment

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

LGTM!

@psrpinto psrpinto merged commit 7bad7b4 into trunk Dec 13, 2024
3 checks passed
@psrpinto psrpinto deleted the ui-using-schema branch December 13, 2024 15:44
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