-
Notifications
You must be signed in to change notification settings - Fork 18
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
Enforce Pricing for Team Sharing #6776
Conversation
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.
Code looks good except for maybe one thing 👍 Didn't test yet, though.
Co-authored-by: Philipp Otto <philippotto@users.noreply.github.com>
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.
Also works well 👍
However, it took me a bit until I could fully "parse" the blurred component. Especially when the blurred area is relatively large, the lock icon is very easy to miss. For example:
I'm not sure how this could be improved. Initially, I had the idea to simply always show the tooltip content next to the lock icon in the center of the blurred area (including the black tooltip background). However, I don't know whether it will look good and will work with the wide teams input that is not very tall. Maybe another color for the lock icon?
Maybe you want to experiment a bit. If it stays as is, it's probably okay, too, since the tooltip will show why things are blurred, too.
By the way, during testing, my Spotify "Discover Weekly" playlist decided to play a song called "Blurry Mess". I'm feeling watched 😅
@@ -123,6 +123,8 @@ export function LayoutMenu(props: LayoutMenuProps) { | |||
autoSaveLayouts, | |||
setAutoSaveLayouts, | |||
saveCurrentLayout, | |||
// rome-ignore lint/correctness/noUnusedVariables: underscore prefix does not work with object destructuring | |||
setCurrentLayout, |
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.
fix for accidentally introduced bug in my recent Linter PR
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.
what was the impact of the bug? I'm asking because I'll create a new release soon-ish.
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.
React/Antd complaining about the setCurrentLayout
prop being present in/leaking into a <Menu>
component, where it was not expected/supported.
@philippotto Please re-review with latest changes:
When testing, please try anonymous access / publicly shared annotation as well. Please also consider how else some of these features can be activated, e.g. through keyboard shortcuts. |
) { | ||
// This function should not be called to check for "Basic" plans since its the default plan for all users anyway. | ||
|
||
if (!organization) return false; |
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.
This needs to be moved below the "is basic" check.
@@ -123,6 +123,8 @@ export function LayoutMenu(props: LayoutMenuProps) { | |||
autoSaveLayouts, | |||
setAutoSaveLayouts, | |||
saveCurrentLayout, | |||
// rome-ignore lint/correctness/noUnusedVariables: underscore prefix does not work with object destructuring | |||
setCurrentLayout, |
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.
what was the impact of the bug? I'm asking because I'll create a new release soon-ish.
Testing went well 👍 One thing I noticed is that if a feature is disabled due to the incorrect orga plan, the user won't know which plan is necessary. For example, for the proofreading tool, one will have to look up the plan comparison to see that the power plan is required. One could mention this in the tooltip. However, it would just be nice to have, I think. |
…_editable_text_style * 'master' of github.com:scalableminds/webknossos: Fix spacing (#6802) Release 23.02.0 (#6801) Fix logo in Readme (#6805) Enforce Pricing for Team Sharing (#6776) applied PR feedback prevent proof reading tool from being unsed through keyboard shortcut disable proof reading tool use isFeatureAllowedByPricingPlan for SecuredRoute enforce pricing for creating precomputed meshes add isFeatureAllowedByPricingPlan helper function re-styled pricing alert Update frontend/javascripts/components/pricing_enforcers.tsx updated changelog add new blur component around certain paid features
PR adds a new wrapper component blur components/features that are not part of an organization's selected plan.
Changes:
PricingPlanEnum
topricing_plan_utils
to unroll a cyclic dependencyURL of deployed dev instance (used for testing):
Steps to test:
Issues:
(Please delete unneeded items, merge only when none are left open)