-
Notifications
You must be signed in to change notification settings - Fork 2.6k
Remove UpsellPageComponent #19048
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
Remove UpsellPageComponent #19048
Conversation
8b74605
to
6a82acd
Compare
6a82acd
to
ca46d04
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The changes look good overall. The only issue I found is some inconsistnecy with regards to showing a PageHeader or not. I'd be in favour of showing the PageHeaders now. Those are the pages I found which are currently missing the PageHeader:
- Notification/Date alerts
- Notification/Sharing
- OneDrive/SharePoint integration
- PlaceholderUsers (see my other comment)
- WP/Sharing queries (might be out of scope since they don't even have a pageHeader yet)
Further, the routes.rb
file can be cleaned up a bit, now that we do not have the upsell routes any more.
6d26a67
to
fe8b55d
Compare
fe8b55d
to
a86fc24
Compare
No longer in use except one-drive
This only works if the ID maps randomly.
a86fc24
to
9901050
Compare
Ticket
https://community.openproject.org/projects/gmbh/work_packages/64212/activity
What are you trying to accomplish?
guard_enterprise_feature
helper as discussed in the dev open doorNote: Not all pages have their upsell flow removed completely. For example, onedrive/sharepoint still refers to a separate landing page. We can discuss separately if a trial banner is required here, and how the page should behave like in this case.
Merge checklist