Skip to content

Pages POC #4396

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

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

Pages POC #4396

wants to merge 164 commits into from

Conversation

jespino
Copy link
Contributor

@jespino jespino commented Dec 23, 2022

Summary

Pages POC

Pending work

The most outstanding pending work here is to stabilize the changes in the router, I'm no longer using a new router for pages/boards, I'm using the main router and generating the prefix for the routes based on the urls needed to be generated, none for standalone/app mode, and /boards and /pages for boards and pages itself.

This is not working great, there are some problems specially in team switch while you are in pages/boards, but I think I can sort it out sooner than later.

jespino and others added 30 commits August 24, 2022 14:16
@jespino jespino added 2: Dev Review Requires review by a core committer Do Not Merge Should not be merged until this label is removed labels Dec 23, 2022
@@ -0,0 +1,3 @@
{{- /* addColumnIfNeeded tableName columnName datatype constraint */ -}}
{{ addColumnIfNeeded "boards" "is_pages_folder" "boolean" "false"}}
Copy link
Member

Choose a reason for hiding this comment

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

The last parameter should be "default false" not just false. The current command generates syntax error.

Copy link
Collaborator

@sbishel sbishel left a comment

Choose a reason for hiding this comment

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

Wow...huge! I didn't see anything egregious, but kept loosing myself. One thing not clear to me from the code is
category -> board -> page(s)
Seems like an extra layer in there. But probably just not clear.

Also, before merging I think we should bug bash both with the feature flag off and on. Let's get a couple spinwicks up and try to bug bash (ff off) next week.

@sbishel sbishel requested a review from wiggin77 January 19, 2023 20:30
Copy link
Contributor

@asaadmahmood asaadmahmood left a comment

Choose a reason for hiding this comment

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

Not sure if and when we'll be doing this, so taking this off of my PR to review list by requesting a review.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2: Dev Review Requires review by a core committer Do Not Merge Should not be merged until this label is removed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants