Skip to content

Add brand tests #1675

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

Merged
merged 1 commit into from
Mar 25, 2025
Merged

Add brand tests #1675

merged 1 commit into from
Mar 25, 2025

Conversation

Kitenite
Copy link
Contributor

@Kitenite Kitenite commented Mar 25, 2025

Description

Related Issues

Type of Change

  • Bug fix
  • New feature
  • Documentation update
  • Release
  • Refactor
  • Other (please describe):

Testing

Screenshots (if applicable)

Additional Notes


Important

Add test cases for camelCase and update UI defaults in BrandTab and LayersPanel.

  • Tests:
    • Add test cases for camelCase in helpers.test.ts to handle UPPERCASE and Capitalized inputs.
  • UI Changes:
    • Set showColorPanel default to true in BrandTab.
    • Hide button in LayersPanel for APPS tab when not selected.

This description was created by Ellipsis for 7e8b413. It will automatically update as commits are pushed.

@Kitenite Kitenite merged commit f048046 into main Mar 25, 2025
@Kitenite Kitenite deleted the bugs/brand-test branch March 25, 2025 20:40
Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

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

👍 Looks good to me! Reviewed everything up to 7e8b413 in 1 minute and 12 seconds

More details
  • Looked at 44 lines of code in 3 files
  • Skipped 0 files when reviewing.
  • Skipped posting 7 drafted comments based on config settings.
1. apps/studio/src/routes/editor/LayersPanel/BrandTab/index.tsx:29
  • Draft comment:
    Default value for showColorPanel changed from false to true. Confirm if this behavior is intended as it always shows the ColorPanel on Brand tab load.
  • Reason this comment was not posted:
    Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50%
    This comment violates our rules because it's asking the author to "confirm if this behavior is intended." We should assume the author intentionally made this UI change. The change is straightforward - it just changes which panel is shown by default. This is a pure UI/UX decision that doesn't affect logic.
    Maybe this change could affect user experience significantly by showing a different default view? Maybe there are performance implications of loading the ColorPanel first?
    Even if there are UX implications, our rules explicitly state not to comment on UI changes - we should assume the author made this choice deliberately. The performance impact would be negligible since both panels exist in the code regardless.
    This comment should be deleted as it violates our rules by questioning a UI change and asking the author to confirm their intention.
2. apps/studio/src/routes/editor/LayersPanel/BrandTab/index.tsx:29
  • Draft comment:
    PR description mentions onboarding docs update, but changes here affect UI behavior. Ensure that the PR addresses the documented issue requirements.
  • Reason this comment was not posted:
    Comment did not seem useful. Confidence is useful = 0% <= threshold 50%
    This comment is asking the PR author to ensure that the PR addresses the documented issue requirements, which is similar to asking them to confirm their intention or ensure the behavior is intended. This violates the rules against asking the author to confirm or ensure things.
3. apps/studio/src/routes/editor/LayersPanel/index.tsx:175
  • Draft comment:
    APPS tab button is now given a 'hidden' class. Verify that this is intended and that hiding the APPS tab is consistent with feature requirements.
  • Reason this comment was not posted:
    Comment did not seem useful. Confidence is useful = 0% <= threshold 50%
    The comment is asking the PR author to verify their intention, which violates the rules. It does not provide a specific code suggestion or ask for a specific test to be written. Therefore, it should be removed.
4. apps/studio/tests/helpers.test.ts:83
  • Draft comment:
    Added tests for converting UPPERCASE and Capitalized strings using camelCase. These tests enhance coverage and seem appropriate.
  • Reason this comment was not posted:
    Confidence changes required: 0% <= threshold 50%
    None
5. apps/studio/src/routes/editor/LayersPanel/BrandTab/index.tsx:29
  • Draft comment:
    The initial state for showColorPanel is now true, so the ColorPanel always displays on load. Confirm if this change is intentional.
  • Reason this comment was not posted:
    Confidence changes required: 33% <= threshold 50%
    None
6. apps/studio/src/routes/editor/LayersPanel/index.tsx:175
  • Draft comment:
    The APPS tab button now includes a 'hidden' class. If this feature is disabled temporarily, consider conditionally rendering it instead of keeping event handlers attached.
  • Reason this comment was not posted:
    Confidence changes required: 33% <= threshold 50%
    None
7. apps/studio/tests/helpers.test.ts:83
  • Draft comment:
    Additional tests for converting UPPERCASE and Capitalized strings in camelCase are added. Verify these cases match the intended behavior of lodash's camelCase.
  • Reason this comment was not posted:
    Confidence changes required: 33% <= threshold 50%
    None

Workflow ID: wflow_upOYuw2QPITAlCwZ


You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

zongkelong pushed a commit to zongkelong/coolook that referenced this pull request Mar 27, 2025
…slation_zh

* 'main' of https://github.com/onlook-dev/onlook:
  Add brand tests (onlook-dev#1675)
  Disable elide lines for precommit hooks (onlook-dev#1674)
  App Panel, Installed Apps, App Details (onlook-dev#1608)
  Modifying configs for linux packages (onlook-dev#1664)
  fix: prevent duplicate variable in tailwind (onlook-dev#1671)
ml-delaurier pushed a commit to ml-delaurier/nolook that referenced this pull request Apr 23, 2025
t1c1 pushed a commit to t1c1/onlookbotcodes that referenced this pull request Jun 5, 2025
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.

1 participant