Skip to content

Conversation

@oguzkocer
Copy link
Contributor

Follows established patterns to implement /navigation endpoint.

I initially implemented it as a separate endpoint, but then @jkmassel and I discussed it and decided to make it a PostEndpointType. We do lose a bit static type information with this approach, so I kept the original implementation as well, so we can decide in this PR whether to keep the PostEndpointType approach or use a separate endpoint. Either way, I think it's good to have both approaches in the git history to be able to revert our decision if we regret it.

I haven't implemented Kotlin integration tests yet as I'd like to make this ^ decision first.

Copy link
Contributor

@jkmassel jkmassel left a comment

Choose a reason for hiding this comment

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

I tested this and it works fine.

The enum wrappers for title and content are a bit weird, but it's workable. I think having these post params be optional make a lot of sense in the context of custom post types, so I think this approach works.

I'm good to move forward with it if you are.

@oguzkocer
Copy link
Contributor Author

The enum wrappers for title and content are a bit weird, but it's workable. I think having these post params be optional make a lot of sense in the context of custom post types, so I think this approach works.

@jkmassel As mentioned in the PR description, we won't actually use the types in navigations module if we are going with the PostEndpointType implementation. They are removed in 5b887e4.

@oguzkocer oguzkocer marked this pull request as ready for review October 3, 2025 18:24
@oguzkocer oguzkocer enabled auto-merge (squash) October 3, 2025 18:24
@oguzkocer oguzkocer merged commit c5cffed into trunk Oct 3, 2025
22 checks passed
@oguzkocer oguzkocer deleted the navigations branch October 3, 2025 22:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants