-
Notifications
You must be signed in to change notification settings - Fork 154
Add dynamic header title and version for Pro mode #1753
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
base: develop
Are you sure you want to change the base?
Add dynamic header title and version for Pro mode #1753
Conversation
Introduces computed properties to display 'WP User Frontend Pro' and the Pro version when the isPro prop and Pro activation are true. Falls back to the standard title and version otherwise, improving header context for both free and Pro users.
WalkthroughHeader.vue now imports Vue's Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
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.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
assets/js/components/Header.vue(1 hunks)
🔇 Additional comments (1)
assets/js/components/Header.vue (1)
37-38: Good separation of display logic into computed header bindingsUsing
headerTitleandheaderVersionin the template instead of hardcoded strings/values keeps the markup clean and centralizes the Pro/free display logic in one place. This will make future changes to naming/version rules easier.
sapayth
left a comment
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.
when Pro is active, we need to show WP User Frontend Pro on every pages header
The isPro prop is no longer needed as the logic now relies solely on the global wpuf.isProActive flag. This simplifies the component's API and reduces redundancy.
done @sapayth vai |
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.
Actionable comments posted: 1
🧹 Nitpick comments (1)
assets/js/components/Header.vue (1)
19-21: Consider internationalizing the Pro title string.The string 'WP User Frontend Pro' is hard-coded. While brand names often don't require translation, consider wrapping it with
__()for consistency with other user-facing strings in the component, or add a comment explaining why it's excluded from i18n.Apply this diff to internationalize the string:
const headerTitle = computed(() => { - return wpuf.isProActive ? 'WP User Frontend Pro' : 'WP User Frontend'; + return wpuf.isProActive ? __('WP User Frontend Pro', 'wp-user-frontend') : __('WP User Frontend', 'wp-user-frontend'); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
assets/js/components/Header.vue(2 hunks)
🔇 Additional comments (3)
assets/js/components/Header.vue (3)
3-3: LGTM!The import of
computedfrom Vue is correct and necessary for the computed properties introduced below.
23-25: LGTM! Pro activation check is correctly aligned.The version logic now correctly checks
wpuf.isProActivealongsidewpuf.pro_version, which addresses the concern from the previous review about ensuring version and title logic remain consistent. The fallback towpuf.versionprovides appropriate default behavior.
32-33: LGTM!The template correctly binds the computed properties using Vue interpolation syntax. The header title and version will now dynamically reflect the Pro status.
| const headerTitle = computed(() => { | ||
| return wpuf.isProActive ? 'WP User Frontend Pro' : 'WP User Frontend'; | ||
| }); |
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.
Clarify discrepancy between PR description and implementation.
The PR description states the header should display Pro title "when the isPro prop and Pro activation are true," but the implementation only checks wpuf.isProActive without an isPro prop. The component's props definition (lines 5-11) doesn't include isPro.
If relying solely on wpuf.isProActive is intentional, consider updating the PR description to reflect the actual implementation.
🤖 Prompt for AI Agents
assets/js/components/Header.vue lines 19-21: the computed headerTitle only
checks wpuf.isProActive but the PR description mentions an isPro prop that
doesn't exist in the component props (lines 5-11); either add an isPro boolean
prop and include it in the computed (e.g., check both props.isPro and
wpuf.isProActive) or, if the intention is to rely solely on wpuf.isProActive,
update the PR description to remove mention of an isPro prop and clarify that
only wpuf.isProActive controls the Pro title.
Introduces computed properties to display 'WP User Frontend Pro' and the Pro version when the isPro prop and Pro activation are true. Falls back to the standard title and version otherwise, improving header context for both free and Pro users. Related PRO PR
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.