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.

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

BA-2403: logo override for native #247

Open
wants to merge 10 commits into
base: master
Choose a base branch
from
Open

Conversation

pt-tsl
Copy link
Contributor

@pt-tsl pt-tsl commented Mar 27, 2025

Acceptance Criteria
Context
Making it easy to change the BaseApp logo to a prospect client's logo in RN. That will allow different theme visualizations for prospect's demos.

Given the need to change the logo on demand.
When the feature is implemented.
Then it should support the following functionalities:

Dynamic Logo Display:

  • create/use the Theme section inside the user's settings page and build the Logo section there
  • create image inputs for normal logo and condensed logos
    • the inputs show the current logo as the preview image
    • it should accept only SVG formats
  • after the image upload and saving, we should convert the SVG to base64 and store it in the local storage
  • ensure all static logo images use the local storage base64 image as the first option; if it's not found, use the static image
  • the logo change is meant to be temporary, meaning that if the person didn't upload any image, the app shall render the static image instead
  • the change should be only applicable to the user session (e.g., someone accessing the site from a different computer should not be able to detect the logo change)
  • only superusers will have permissions to change any logo image

Design Link: https://www.figma.com/design/z5ZdVlKEMmGIX4GVnjwfxA/BaseApp---NATIVE?node-id=5677-33215&t=U1EqoGJmE4cUX2wp-0

Approvd
https://app.approvd.io/projects/BA/stories/40111

Summary by CodeRabbit

  • New Features

    • Enhanced button components with dynamic text styling and additional mode options.
    • Added a new vector icon for edit actions.
    • Enabled native logo customization with persistent override capabilities.
    • Introduced a new hook for image selection from the photo library or camera.
  • Bug Fixes

    • Improved clarity in text displayed for image and banner removal actions.
  • Chores

    • Updated component, design system, and wagtail packages to the latest versions.
    • Introduced asynchronous storage support through dependency updates.

Copy link

changeset-bot bot commented Mar 27, 2025

⚠️ No Changeset found

Latest commit: 78b0fa6

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

Copy link

coderabbitai bot commented Mar 27, 2025

Walkthrough

This pull request updates multiple packages and modules. The test for the useChangePassword hook now uses a new password value. Version numbers have been bumped for both the components and design system packages, with dependency updates reflected in changelogs and package.json files. The design system now features a new dynamic styling approach for button text and introduces new hooks and types for logo overrides. Additionally, obsolete TODO comments have been removed from logo components, and the pnpm workspace now includes the async-storage dependency.

Changes

File(s) Change Summary
packages/authentication/.../useChangePassword.test.tsx Updated password variable from '12345#Abcde' to 'abcABC@123456' in both success and error test scenarios.
packages/components/{CHANGELOG.md, package.json} Bumped version from 1.0.28 to 1.0.29 and added a dependency update entry in the changelog.
packages/design-system/{CHANGELOG.md, package.json} Bumped version from 1.0.12 to 1.0.13, added new feature useLogoOverride, and introduced @react-native-async-storage/async-storage dependency.
packages/design-system/components/native/buttons/Button/{index.tsx, styles.ts, types.ts} Refactored button component: removed static textStyles, added dynamic createTextStyles, and extended ButtonStylesOptions with an optional mode property.
packages/design-system/components/native/icons/{VectorEditIcon/index.tsx, index.ts} Added a new VectorEditIcon component and updated the export in the icons index.
packages/design-system/components/web/logos/{CustomLogoCondensed, Logo, ProjectLogo}/index.tsx Removed TODO comments regarding future relocation of logo components.
packages/design-system/hooks/native/{index.ts, useLogoOverrides/constants.ts, useLogoOverrides/index.ts, useLogoOverrides/types.ts} Introduced the new useLogoOverrides hook along with its constants and types to manage logo overrides using AsyncStorage.
packages/wagtail/{CHANGELOG.md, package.json} Bumped version from 1.0.27/1.0.10 to 1.0.28 and updated dependency on the design system package.
pnpm-workspace.yaml Added the @react-native-async-storage/async-storage dependency (^2.1.2) to the react-native-core catalog.

Suggested labels

needs changes

Suggested reviewers

  • tsl-ps2

Poem

I'm a bunny with a hop and a click,
Testing passwords, making changes quick.
Versions rise and hooks now play,
New styles shine in a dynamic way.
With async dreams and logos bright,
I celebrate our code with pure delight!
🐰💻 Hop along in the magic of light!

Warning

There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure.

🔧 ESLint

If the error stems from missing dependencies, add them to the package.json file. For unrecoverable errors (e.g., due to private dependencies), disable the tool in the CodeRabbit configuration.

packages/components/modules/profiles/native/ProfileSettingsComponent/BottomDrawer/index.tsx

Oops! Something went wrong! :(

ESLint: 8.57.1

Error: Cannot read config file: /packages/components/.eslintrc.js
Error: Cannot find module '@baseapp-frontend/config/.eslintrc-with-restricted-paths.js'
Require stack:

  • /packages/components/.eslintrc.js
  • /node_modules/.pnpm/@eslint+eslintrc@2.1.4/node_modules/@eslint/eslintrc/dist/eslintrc.cjs
  • /node_modules/.pnpm/eslint@8.57.1/node_modules/eslint/lib/cli-engine/cli-engine.js
  • /node_modules/.pnpm/eslint@8.57.1/node_modules/eslint/lib/eslint/eslint.js
  • /node_modules/.pnpm/eslint@8.57.1/node_modules/eslint/lib/eslint/index.js
  • /node_modules/.pnpm/eslint@8.57.1/node_modules/eslint/lib/cli.js
  • /node_modules/.pnpm/eslint@8.57.1/node_modules/eslint/bin/eslint.js
    at Module._resolveFilename (node:internal/modules/cjs/loader:1248:15)
    at Module._load (node:internal/modules/cjs/loader:1074:27)
    at TracingChannel.traceSync (node:diagnostics_channel:315:14)
    at wrapModuleLoad (node:internal/modules/cjs/loader:217:24)
    at Module.require (node:internal/modules/cjs/loader:1339:12)
    at require (node:internal/modules/helpers:135:16)
    at Object. (/packages/components/.eslintrc.js:1:18)
    at Module._compile (node:internal/modules/cjs/loader:1546:14)
    at Module._extensions..js (node:internal/modules/cjs/loader:1691:10)
    at Module.load (node:internal/modules/cjs/loader:1317:32)
packages/authentication/modules/access/useChangePassword/__tests__/useChangePassword.test.tsx

Oops! Something went wrong! :(

ESLint: 8.57.1

Error: Cannot read config file: /packages/authentication/.eslintrc.js
Error: Cannot find module '@baseapp-frontend/config/.eslintrc.js'
Require stack:

  • /packages/authentication/.eslintrc.js
  • /node_modules/.pnpm/@eslint+eslintrc@2.1.4/node_modules/@eslint/eslintrc/dist/eslintrc.cjs
  • /node_modules/.pnpm/eslint@8.57.1/node_modules/eslint/lib/cli-engine/cli-engine.js
  • /node_modules/.pnpm/eslint@8.57.1/node_modules/eslint/lib/eslint/eslint.js
  • /node_modules/.pnpm/eslint@8.57.1/node_modules/eslint/lib/eslint/index.js
  • /node_modules/.pnpm/eslint@8.57.1/node_modules/eslint/lib/cli.js
  • /node_modules/.pnpm/eslint@8.57.1/node_modules/eslint/bin/eslint.js
    at Module._resolveFilename (node:internal/modules/cjs/loader:1248:15)
    at Module._load (node:internal/modules/cjs/loader:1074:27)
    at TracingChannel.traceSync (node:diagnostics_channel:315:14)
    at wrapModuleLoad (node:internal/modules/cjs/loader:217:24)
    at Module.require (node:internal/modules/cjs/loader:1339:12)
    at require (node:internal/modules/helpers:135:16)
    at Object. (/packages/authentication/.eslintrc.js:1:18)
    at Module._compile (node:internal/modules/cjs/loader:1546:14)
    at Module._extensions..js (node:internal/modules/cjs/loader:1691:10)
    at Module.load (node:internal/modules/cjs/loader:1317:32)
packages/components/modules/profiles/native/ProfileSettingsComponent/BottomDrawer/types.ts

Oops! Something went wrong! :(

ESLint: 8.57.1

Error: Cannot read config file: /packages/components/.eslintrc.js
Error: Cannot find module '@baseapp-frontend/config/.eslintrc-with-restricted-paths.js'
Require stack:

  • /packages/components/.eslintrc.js
  • /node_modules/.pnpm/@eslint+eslintrc@2.1.4/node_modules/@eslint/eslintrc/dist/eslintrc.cjs
  • /node_modules/.pnpm/eslint@8.57.1/node_modules/eslint/lib/cli-engine/cli-engine.js
  • /node_modules/.pnpm/eslint@8.57.1/node_modules/eslint/lib/eslint/eslint.js
  • /node_modules/.pnpm/eslint@8.57.1/node_modules/eslint/lib/eslint/index.js
  • /node_modules/.pnpm/eslint@8.57.1/node_modules/eslint/lib/cli.js
  • /node_modules/.pnpm/eslint@8.57.1/node_modules/eslint/bin/eslint.js
    at Module._resolveFilename (node:internal/modules/cjs/loader:1248:15)
    at Module._load (node:internal/modules/cjs/loader:1074:27)
    at TracingChannel.traceSync (node:diagnostics_channel:315:14)
    at wrapModuleLoad (node:internal/modules/cjs/loader:217:24)
    at Module.require (node:internal/modules/cjs/loader:1339:12)
    at require (node:internal/modules/helpers:135:16)
    at Object. (/packages/components/.eslintrc.js:1:18)
    at Module._compile (node:internal/modules/cjs/loader:1546:14)
    at Module._extensions..js (node:internal/modules/cjs/loader:1691:10)
    at Module.load (node:internal/modules/cjs/loader:1317:32)
  • 12 others
✨ Finishing Touches
  • 📝 Generate Docstrings

🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai plan to trigger planning for file edits and PR creation.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link

@coderabbitai coderabbitai bot left a 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

📥 Commits

Reviewing files that changed from the base of the PR and between 39bf260 and 20c3fa1.

⛔ Files ignored due to path filters (1)
  • pnpm-lock.yaml is excluded by !**/pnpm-lock.yaml
📒 Files selected for processing (20)
  • packages/authentication/modules/access/useChangePassword/__tests__/useChangePassword.test.tsx (1 hunks)
  • packages/components/CHANGELOG.md (1 hunks)
  • packages/components/package.json (1 hunks)
  • packages/design-system/CHANGELOG.md (1 hunks)
  • packages/design-system/components/native/buttons/Button/index.tsx (2 hunks)
  • packages/design-system/components/native/buttons/Button/styles.ts (1 hunks)
  • packages/design-system/components/native/buttons/Button/types.ts (1 hunks)
  • packages/design-system/components/native/icons/VectorEditIcon/index.tsx (1 hunks)
  • packages/design-system/components/native/icons/index.ts (1 hunks)
  • packages/design-system/components/web/logos/CustomLogoCondensed/index.tsx (0 hunks)
  • packages/design-system/components/web/logos/Logo/index.tsx (0 hunks)
  • packages/design-system/components/web/logos/ProjectLogo/index.tsx (0 hunks)
  • packages/design-system/hooks/native/index.ts (1 hunks)
  • packages/design-system/hooks/native/useLogoOverrides/constants.ts (1 hunks)
  • packages/design-system/hooks/native/useLogoOverrides/index.ts (1 hunks)
  • packages/design-system/hooks/native/useLogoOverrides/types.ts (1 hunks)
  • packages/design-system/package.json (2 hunks)
  • packages/wagtail/CHANGELOG.md (1 hunks)
  • packages/wagtail/package.json (1 hunks)
  • pnpm-workspace.yaml (1 hunks)
💤 Files with no reviewable changes (3)
  • packages/design-system/components/web/logos/ProjectLogo/index.tsx
  • packages/design-system/components/web/logos/Logo/index.tsx
  • packages/design-system/components/web/logos/CustomLogoCondensed/index.tsx
🧰 Additional context used
🧬 Code Definitions (2)
packages/design-system/hooks/native/useLogoOverrides/types.ts (1)
packages/design-system/hooks/native/index.ts (2)
  • LogoOverrides (6-6)
  • LogoOverridesKeys (6-6)
packages/design-system/components/native/buttons/Button/index.tsx (1)
packages/design-system/components/native/buttons/Button/styles.ts (1)
  • createTextStyles (47-57)
🔇 Additional comments (25)
packages/design-system/components/native/icons/VectorEditIcon/index.tsx (1)

1-43: Well-implemented SVG icon component for vector editing functionality.

The implementation follows the established pattern for icon components in the design system. The component correctly uses theme colors, handles the active state, and provides sensible defaults for dimensions.

packages/components/package.json (1)

4-4: Version bump reflects dependency updates.

The version has been incremented from 1.0.28 to 1.0.29, which aligns with the changelog updates for dependency changes.

packages/components/CHANGELOG.md (1)

3-8: Properly documented version update in changelog.

The changelog entry correctly documents that version 1.0.29 includes an update to the design-system dependency from version 1.0.11 to 1.0.12.

packages/design-system/components/native/icons/index.ts (1)

27-27: Appropriate export of the new VectorEditIcon component.

The new VectorEditIcon has been correctly added to the exports in the icons index file, making it available for import by other components.

packages/wagtail/package.json (1)

4-4: Version bump aligns with changelog update

The version has been correctly incremented from 1.0.27 to 1.0.28, which is consistent with the changelog entry that follows the standard semantic versioning practice.

packages/wagtail/CHANGELOG.md (1)

3-8: Properly documented dependency update

The changelog entry for version 1.0.28 correctly documents the dependency update to @baseapp-frontend/design-system@1.0.12, which contains the new logo override functionality for native components as described in the PR objectives.

packages/design-system/CHANGELOG.md (1)

3-7: Feature addition properly documented

The changelog entry correctly documents the addition of the useLogoOverride hook for native components, which aligns with the PR objectives to implement dynamic logo customization for React Native.

packages/design-system/hooks/native/index.ts (1)

5-6:

✅ Verification successful

Exports for new logo override functionality

The new exports for the useLogoOverrides hook and its associated types make the logo customization functionality available to other components as required by the PR objectives.


🏁 Script executed:

#!/bin/bash
# Description: Verify the implementation of useLogoOverrides hook and its types

# Check for the useLogoOverrides implementation file
if [ -f "./packages/design-system/hooks/native/useLogoOverrides/index.ts" ]; then
  echo "✅ useLogoOverrides implementation found"
  cat "./packages/design-system/hooks/native/useLogoOverrides/index.ts" | head -n 10
else
  echo "❌ useLogoOverrides implementation not found"
fi

# Check for the types file
if [ -f "./packages/design-system/hooks/native/useLogoOverrides/types.ts" ]; then
  echo "✅ useLogoOverrides types file found"
  cat "./packages/design-system/hooks/native/useLogoOverrides/types.ts" | head -n 10
else
  echo "❌ useLogoOverrides types file not found"
fi

# Check for any constants file that might be used
if [ -f "./packages/design-system/hooks/native/useLogoOverrides/constants.ts" ]; then
  echo "✅ useLogoOverrides constants file found"
  cat "./packages/design-system/hooks/native/useLogoOverrides/constants.ts"
else
  echo "❌ useLogoOverrides constants file not found"
fi

# Look for any usages of the hook in the codebase
echo "Searching for usages of useLogoOverrides in the codebase:"
rg -l "useLogoOverrides" --type ts

Length of output: 1974


Verified: New logo override exports are in place

  • Confirmed that the implementation file (packages/design-system/hooks/native/useLogoOverrides/index.ts) exists and exposes the hook as expected.
  • Verified that the types file (packages/design-system/hooks/native/useLogoOverrides/types.ts) and constants file (packages/design-system/hooks/native/useLogoOverrides/constants.ts) are present and provide the necessary types and defaults.
  • Found multiple usages of useLogoOverrides throughout both native and web components, supporting the intended integration.

Everything appears to meet the PR objectives.

packages/design-system/components/native/buttons/Button/types.ts (1)

13-13: Clean addition of mode property to ButtonStylesOptions

The addition of an optional mode property to ButtonStylesOptions aligns well with the existing type structure, as ButtonMode is already defined and used in the ButtonProps interface. This change enables more flexible styling controls for buttons.

packages/design-system/hooks/native/useLogoOverrides/constants.ts (1)

1-8: Well-structured constants for logo overrides feature

The constants are appropriately defined for the logo override functionality, which aligns with the PR objective of implementing dynamic logo customization. The naming is clear and follows conventions.

This file provides the foundation for storing and retrieving logo overrides, establishing default values that will be used when no custom logos are specified.

packages/design-system/package.json (2)

4-4: Version bump is appropriate

Incrementing the package version from 1.0.11 to 1.0.12 is appropriate for adding the new logo override functionality.


70-70: Good addition of async-storage dependency

The addition of the async-storage dependency is necessary for implementing the logo override feature that persists across app restarts, as mentioned in the PR objectives.

This dependency will allow the logo customizations to be stored in the device's local storage, ensuring that the custom logos remain available until explicitly removed.

pnpm-workspace.yaml (1)

37-37: Correctly added async-storage to workspace catalog

Adding the async-storage package to the react-native-core catalog ensures it's available for use in the design-system package. The version specification looks appropriate.

packages/authentication/modules/access/useChangePassword/__tests__/useChangePassword.test.tsx (1)

14-14: Password value updated to follow stronger pattern

The password has been updated from the previous value to a new format that includes lowercase letters, uppercase letters, special characters, and numbers, making it a more robust example for testing.

packages/design-system/components/native/buttons/Button/index.tsx (2)

8-16: Improved button text styling with dynamic approach

The import statement now includes createTextStyles instead of the static textStyles, which aligns with the goal of making button styling more dynamic.


36-36: Enhanced dynamic text styling based on theme and state

The button's text styling has been refactored to use the new createTextStyles function instead of a static style object. This approach allows for dynamic text styling based on the current theme, color variant, and disabled state.

packages/design-system/components/native/buttons/Button/styles.ts (1)

47-57: Added dynamic text style generation for buttons

This new function createTextStyles follows the same pattern as other style creation functions in the file and provides dynamic text styling based on theme colors, variant type, and disabled state. It correctly handles inheritance and disabled states, ensuring consistent appearance across the application.

packages/design-system/hooks/native/useLogoOverrides/index.ts (2)

1-46: New hook for logo customization supports PR objective

This new hook useLogoOverrides implements the core functionality required for the logo override feature. It:

  1. Uses Jotai with AsyncStorage for persistent state management
  2. Provides methods to get and set logo overrides
  3. Handles SSR environments correctly
  4. Follows React best practices with clean separation of concerns

The implementation aligns perfectly with the PR objectives of facilitating dynamic logo customization for prospect clients in React Native.


10-17:

✅ Verification successful

Verify AsyncStorage implementation for React Native

The implementation uses AsyncStorage for persisting logo overrides, which is appropriate for React Native. However, since this is part of a design system that might be used in different environments, ensure that this dependency works correctly in all target platforms.


🏁 Script executed:

#!/bin/bash
# Check if AsyncStorage is properly imported and used in other native components

# Look for other AsyncStorage imports to verify consistency
rg -A 3 "import AsyncStorage from '@react-native-async-storage/async-storage'" --type typescript

Length of output: 139


Below is a revised approach to verify the AsyncStorage usage. It appears that the previous command failed because the file type flag “typescript” wasn’t recognized. Please run the following new shell script that uses globs to target TypeScript files:

This command should list instances where AsyncStorage is imported along with a few context lines, so you can confirm that its usage is consistent and appropriate for React Native implementations across the codebase. Once you’ve reviewed the results, please verify manually that AsyncStorage behaves correctly across all target platforms.

Action: Re-run the above command and verify AsyncStorage usage in your native components.


🏁 Script executed:

#!/bin/bash
# Search for AsyncStorage import in TypeScript and TSX files using globs
rg -A 3 "import AsyncStorage from '@react-native-async-storage/async-storage'" --glob "*.ts" --glob "*.tsx"

Length of output: 652


Ensure cross-platform storage compatibility

The native hook in
packages/design-system/hooks/native/useLogoOverrides/index.ts
correctly imports and uses AsyncStorage from @react-native-async-storage/async-storage to persist logo overrides on React Native. Since this hook is clearly scoped to the native implementation, its usage appears appropriate.

That said, because our design system might eventually be consumed in environments other than React Native, please ensure that either:

  • An alternative storage solution (or a suitable polyfill) is provided for non-native platforms, or
  • The documentation clearly states that this hook is intended exclusively for React Native usage.
packages/design-system/hooks/native/useLogoOverrides/types.ts (6)

1-4: Well-structured type for logo storage as base64

The LogoOverrideObject type appropriately captures the requirements for storing logos as base64 strings with their MIME types, which aligns with the PR objective of storing SVG images for logo customization.


6-9: Good design for multiple logo variants

The LogoOverrides type effectively supports both normal and square/condensed logo variants as mentioned in the PR objectives. Making the properties optional allows the application to fall back to default static logos when custom ones aren't provided.


11-11: Type-safe key access

Using keyof to create the LogoOverridesKeys type ensures type safety when accessing different logo variants throughout the application.


13-15: Clear state structure

The LogoOverrideState type provides a clean structure for managing logo override settings in the application state.


17-19: Flexible update function signature

The setLogoOverride function accepts a Partial<LogoOverrides> parameter, which allows for updating either or both logo variants independently - a good design choice for usability.


21-21: Well-designed hook return type

The UseLogoOverrides type follows the React hook pattern by combining state and functions, creating a comprehensive interface for the logo override functionality.

Copy link
Collaborator

@tsl-ps2 tsl-ps2 left a comment

Choose a reason for hiding this comment

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

Just to make sure: The scope of this PR was only to create the functionality of logo overrides, but not the actual user interface for it, right?

@pt-tsl pt-tsl requested a review from anicioalexandre March 31, 2025 16:15
@pt-tsl pt-tsl force-pushed the BA-2403-logo-override branch from 20c3fa1 to c588b4c Compare March 31, 2025 16:22
@pt-tsl pt-tsl force-pushed the BA-2403-logo-override branch from c588b4c to 78b0fa6 Compare March 31, 2025 16:23
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 2

🔭 Outside diff range comments (2)
packages/components/modules/profiles/native/ProfileSettingsComponent/index.tsx (1)

1-312: ⚠️ Potential issue

Missing implementation for logo customization requirements.

The PR objectives mention creating a Theme section with Logo inputs for both normal and condensed logos, with SVG format restriction and base64 storage in local storage. This implementation focuses on profile and banner images but doesn't address the logo customization requirements.

Consider adding:

  1. A Theme section in the settings
  2. Logo upload inputs (normal and condensed)
  3. SVG format validation
  4. Base64 conversion and local storage logic
  5. Superuser permission restrictions
#!/bin/bash
# Check if there are any Theme or Logo related components in the codebase
echo "Searching for existing Theme or Logo components..."
rg -i "Theme|Logo" --type ts --type tsx
🧰 Tools
🪛 Biome (1.9.4)

[error] 129-129: Avoid the delete operator which can impact performance.

Unsafe fix: Use an undefined assignment instead.

(lint/performance/noDelete)


[error] 133-133: Avoid the delete operator which can impact performance.

Unsafe fix: Use an undefined assignment instead.

(lint/performance/noDelete)

packages/design-system/hooks/native/useViewPhotoLibrary/index.ts (1)

1-68: 🛠️ Refactor suggestion

Missing base64 conversion and storage functionality.

The PR objectives mention converting SVG logos to base64 and storing them in local storage. While the hook has an isBase64 option, it doesn't implement the storage part.

Consider adding a helper function to handle base64 conversion and storage:

import AsyncStorage from '@react-native-async-storage/async-storage'

// Add to the hook implementation
const storeLogoInLocalStorage = async (
  logoType: 'normal' | 'condensed',
  base64Data: string
) => {
  try {
    const key = logoType === 'normal' ? 'custom_logo' : 'custom_logo_condensed'
    await AsyncStorage.setItem(key, base64Data)
    return true
  } catch (error) {
    sendToast('Failed to save logo', { type: 'error' })
    return false
  }
}

// Then call this in handleImageSelection when isBase64 is true
if (isBase64 && loadedImage.base64) {
  storeLogoInLocalStorage(type === 'image' ? 'normal' : 'condensed', loadedImage.base64)
}

Also, consider adding functionality to retrieve stored logos:

// Function to get stored logo
const getStoredLogo = async (logoType: 'normal' | 'condensed') => {
  try {
    const key = logoType === 'normal' ? 'custom_logo' : 'custom_logo_condensed'
    return await AsyncStorage.getItem(key)
  } catch (error) {
    return null
  }
}
🧹 Nitpick comments (5)
packages/design-system/components/native/buttons/Button/styles.ts (1)

47-57: LGTM! The new function enhances button text styling flexibility.

The addition of createTextStyles follows the established pattern of the other style creator functions in this file, allowing dynamic text color based on theme, variant, and disabled state. This change helps support the dynamic styling needed for the logo override feature.

Consider adding a brief comment explaining the purpose of this function for consistency with other parts of the codebase.

packages/components/modules/profiles/native/ProfileSettingsComponent/BottomDrawer/index.tsx (1)

54-54: Make text handling more maintainable

The conditional text logic works, but it would be more maintainable to use a mapping object or match exactly to what's in the types definition.

-            Remove {type === 'image' ? 'image' : 'banner'}
+            Remove {type === 'image' ? 'image' : 'banner'}

Consider using a more explicit approach that would be easier to maintain if more types are added in the future:

-            Remove {type === 'image' ? 'image' : 'banner'}
+            Remove {type === 'bannerImage' ? 'banner' : type}

Or even better, use a mapping object:

const typeDisplayNames = {
  image: 'image',
  bannerImage: 'banner'
};

And then:

-            Remove {type === 'image' ? 'image' : 'banner'}
+            Remove {typeDisplayNames[type] || type}
packages/design-system/hooks/native/useLogoOverrides/index.ts (1)

1-2: Remove unnecessary 'use client' directive

The 'use client' directive is specific to Next.js applications and is used to indicate client-side rendering. Since this is a React Native application and there is no server-side rendering context, this directive is unnecessary.

-'use client'
-
packages/design-system/hooks/native/useViewPhotoLibrary/types.ts (1)

1-12: Well-defined type interface

The ViewPhotoLibraryProps interface is well-structured and provides good type safety for the useViewPhotoLibrary hook. The types align with the changes made in other components, including the 'bannerImage' type which replaces 'banner'.

One minor suggestion - since the type parameter in onResult matches the required type property, you could consider using the same type definition to avoid duplication:

+type PhotoLibraryType = 'default' | 'square' | 'image' | 'bannerImage'

export interface ViewPhotoLibraryProps {
  onResult: (
    image?: ImagePicker.ImagePickerAsset,
-    type?: 'default' | 'square' | 'image' | 'bannerImage',
+    type?: PhotoLibraryType,
  ) => void
  onFinally?: () => void
  allowsEditing: boolean
  isBase64: boolean
-  type: 'default' | 'square' | 'image' | 'bannerImage'
+  type: PhotoLibraryType
}

This would make future updates more maintainable as you'd only need to change the type definition in one place.

packages/design-system/hooks/native/useViewPhotoLibrary/index.ts (1)

30-44: Good permission handling for camera and photo library.

The implementation correctly requests permissions and provides user feedback when permissions are denied. However, the quality is set to 1 (maximum), which could result in unnecessarily large files.

Consider using a more balanced quality setting:

  const result = await ImagePicker.launchCameraAsync({
    allowsEditing,
    aspect: [aspectRatio[0], aspectRatio[1]],
-   quality: 1,
+   quality: 0.8,
  })

And similarly for the image library:

  const image = await ImagePicker.launchImageLibraryAsync({
    mediaTypes: 'images',
    base64: isBase64,
    allowsEditing,
    aspect: [aspectRatio[0], aspectRatio[1]],
-   quality: 1,
+   quality: 0.8,
  })

Also applies to: 46-62

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 20c3fa1 and 78b0fa6.

⛔ Files ignored due to path filters (1)
  • pnpm-lock.yaml is excluded by !**/pnpm-lock.yaml
📒 Files selected for processing (25)
  • packages/authentication/modules/access/useChangePassword/__tests__/useChangePassword.test.tsx (1 hunks)
  • packages/components/CHANGELOG.md (1 hunks)
  • packages/components/modules/profiles/native/ProfileSettingsComponent/BottomDrawer/index.tsx (1 hunks)
  • packages/components/modules/profiles/native/ProfileSettingsComponent/BottomDrawer/types.ts (1 hunks)
  • packages/components/modules/profiles/native/ProfileSettingsComponent/index.tsx (5 hunks)
  • packages/components/package.json (1 hunks)
  • packages/design-system/CHANGELOG.md (1 hunks)
  • packages/design-system/components/native/buttons/Button/index.tsx (2 hunks)
  • packages/design-system/components/native/buttons/Button/styles.ts (1 hunks)
  • packages/design-system/components/native/buttons/Button/types.ts (1 hunks)
  • packages/design-system/components/native/icons/VectorEditIcon/index.tsx (1 hunks)
  • packages/design-system/components/native/icons/index.ts (1 hunks)
  • packages/design-system/components/web/logos/CustomLogoCondensed/index.tsx (0 hunks)
  • packages/design-system/components/web/logos/Logo/index.tsx (0 hunks)
  • packages/design-system/components/web/logos/ProjectLogo/index.tsx (0 hunks)
  • packages/design-system/hooks/native/index.ts (1 hunks)
  • packages/design-system/hooks/native/useLogoOverrides/constants.ts (1 hunks)
  • packages/design-system/hooks/native/useLogoOverrides/index.ts (1 hunks)
  • packages/design-system/hooks/native/useLogoOverrides/types.ts (1 hunks)
  • packages/design-system/hooks/native/useViewPhotoLibrary/index.ts (1 hunks)
  • packages/design-system/hooks/native/useViewPhotoLibrary/types.ts (1 hunks)
  • packages/design-system/package.json (2 hunks)
  • packages/wagtail/CHANGELOG.md (1 hunks)
  • packages/wagtail/package.json (1 hunks)
  • pnpm-workspace.yaml (1 hunks)
💤 Files with no reviewable changes (3)
  • packages/design-system/components/web/logos/ProjectLogo/index.tsx
  • packages/design-system/components/web/logos/Logo/index.tsx
  • packages/design-system/components/web/logos/CustomLogoCondensed/index.tsx
🚧 Files skipped from review as they are similar to previous changes (15)
  • packages/components/package.json
  • packages/design-system/components/native/buttons/Button/types.ts
  • packages/components/CHANGELOG.md
  • packages/wagtail/CHANGELOG.md
  • packages/design-system/package.json
  • packages/design-system/hooks/native/useLogoOverrides/constants.ts
  • packages/design-system/components/native/buttons/Button/index.tsx
  • packages/wagtail/package.json
  • packages/authentication/modules/access/useChangePassword/tests/useChangePassword.test.tsx
  • pnpm-workspace.yaml
  • packages/design-system/CHANGELOG.md
  • packages/design-system/components/native/icons/index.ts
  • packages/design-system/components/native/icons/VectorEditIcon/index.tsx
  • packages/design-system/hooks/native/index.ts
  • packages/design-system/hooks/native/useLogoOverrides/types.ts
🧰 Additional context used
🧬 Code Definitions (2)
packages/design-system/hooks/native/useViewPhotoLibrary/types.ts (1)
packages/design-system/hooks/native/index.ts (1)
  • ViewPhotoLibraryProps (9-9)
packages/design-system/hooks/native/useViewPhotoLibrary/index.ts (2)
packages/design-system/hooks/native/index.ts (1)
  • ViewPhotoLibraryProps (9-9)
packages/design-system/hooks/native/useViewPhotoLibrary/types.ts (1)
  • ViewPhotoLibraryProps (3-12)
🔇 Additional comments (8)
packages/components/modules/profiles/native/ProfileSettingsComponent/BottomDrawer/types.ts (1)

6-6: Type update looks good

The update from 'banner' to 'bannerImage' creates more consistency in naming conventions, matching the changes made in the ProfileSettingsComponent.

Also applies to: 9-9

packages/design-system/hooks/native/useLogoOverrides/index.ts (1)

3-37: Well-structured hook for logo overrides

The implementation of the useLogoOverrides hook is clean and effective. It:

  • Uses Jotai with AsyncStorage for persisting logo settings
  • Correctly handles async state updates with proper promise handling
  • Provides a clean API for getting and setting logo overrides

This is a good implementation that aligns with the PR objective of enabling dynamic logo customization.

packages/components/modules/profiles/native/ProfileSettingsComponent/index.tsx (4)

7-7: Good addition of the useViewPhotoLibrary hook.

This hook provides a cleaner abstraction for image selection functionality, centralizing the permission handling and image picker logic.


45-45: Improved type naming consistency.

Changing from 'banner' to 'bannerImage' provides better consistency with the actual field names used throughout the component.


182-189: Improved type safety for handleRemoveImage.

The function parameter type has been properly updated to match the new fieldType state, ensuring type consistency throughout the component.


202-204: Consistent field type naming.

Changed from 'banner' to 'bannerImage' to maintain consistency with the state definition.

packages/design-system/hooks/native/useViewPhotoLibrary/index.ts (2)

7-13: Well-structured hook with clear props.

The hook accepts appropriate props that make it flexible and reusable across different image selection scenarios.


15-15: Good dynamic aspect ratio handling.

Setting the aspect ratio based on image type provides appropriate constraints for different use cases. Square (1:1) for profile images and widescreen (16:9) for banner images is a sensible default.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants