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.

Sign up for GitHub

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

Jump to bottom

feat(): articles need fields like in the section pages for sdi zones #529

Closed
wants to merge 1 commit into from
Closed

feat(): articles need fields like in the section pages for sdi zones #529

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 4 additions & 0 deletions stroeer/page/article/v1/article_page.proto
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ package stroeer.page.article.v1;
import "stroeer/core/v1/article.proto";
import "stroeer/core/v1/shared.proto";
import "stroeer/page/stage/v1/stage.proto";
import "stroeer/page/section/v1/section_page.proto";

option go_package = "github.com/stroeer/go-tapir/page/article/v1;article";

Expand All @@ -28,4 +29,7 @@ message ArticlePage {
// - submenu
// - external references
stroeer.core.v1.Reference navigation_menu = 4;

// meta information to render an article page such as commercial settings.
stroeer.page.section.v1.SectionPage.Section section = 5;
Comment on lines +33 to +34
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks wrong to me. The equivalent of SectionPage.Section.fields is ArticlePage.Article.fields

Copy link
Contributor

Choose a reason for hiding this comment

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

that is true, and I also thought about just adding the fields, but since there are efforts to make ArticlePage and SectionPage models more similar, I preferred to use the whole Section field.
@bobaaaaa

Copy link
Contributor

@bobaaaaa bobaaaaa Apr 18, 2024 •
edited

Choose a reason for hiding this comment

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

I know the issue. We need to deal with that in our FE too. I created an interface around ArticlePage and SectionPage for that reason.

Still: this change is wrong to me because of duplicate data. E.g. ArticlePage.article.section_tree & ArticlePage.section.section_tree

Copy link
Author

Choose a reason for hiding this comment

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

To quote Imogen Heap: "Where do we go from here":
So just the fields than or is something wrong with that too? Since we want to keep sections and articles more alike.

}