Skip to content

Nicole/colorpicker#524

Draft
nicoledbelcher wants to merge 6 commits intocoinbase:masterfrom
nicoledbelcher:nicole/colorpicker
Draft

Nicole/colorpicker#524
nicoledbelcher wants to merge 6 commits intocoinbase:masterfrom
nicoledbelcher:nicole/colorpicker

Conversation

@nicoledbelcher
Copy link

What changed? Why?

Root cause (required for bugfixes)

UI changes

iOS Old iOS New
old screenshot new screenshot
Android Old Android New
old screenshot new screenshot
Web Old Web New
old screenshot new screenshot

Testing

How has it been tested?

  • Unit tests
  • Interaction tests
  • Pseudo State tests
  • Manual - Web
  • Manual - Android (Emulator / Device)
  • Manual - iOS (Emulator / Device)

Testing instructions

Illustrations/Icons Checklist

Required if this PR changes files under packages/illustrations/** or packages/icons/**

  • verified visreg changes with Terran (include link to visreg run/approval)
  • all illustration/icons names have been reviewed by Dom and/or Terran

Change management

type=routine
risk=low
impact=sev5

automerge=false

Adds ColorPairingTool to docs site, color-pairing utilities and components to vite-app, cds-finder app, guidelines documentation, and various supporting updates.

Made-with: Cursor
…heming

- Remove Color matching / Color pairs tab strip; single-flow layout
- Scope LineChart scrubber overlay/line CSS vars to chart card for light playground
- Hue-aware primitive matching and related adjustments

Made-with: Cursor
@cb-heimdall
Copy link
Collaborator

cb-heimdall commented Mar 20, 2026

🟡 Heimdall Review Status

Requirement Status More Info
Reviews 🟡 0/1
Denominator calculation
Show calculation
1 if user is bot 0
1 if user is external 0
2 if repo is sensitive 0
From .codeflow.yml 1
Additional review requirements
Show calculation
Max 0
0
From CODEOWNERS 1
Global minimum 0
Max 1
1
1 if commit is unverified 0
Sum 1
CODEOWNERS 🟡 See below

🟡 CODEOWNERS

Code Owner Status Calculation
ui-systems-eng-team 🟡 0/1
Denominator calculation
Additional CODEOWNERS Requirement
Show calculation
Sum 0
0
From CODEOWNERS 1
Sum 1

@@ -0,0 +1,44 @@
---
Copy link
Contributor

Choose a reason for hiding this comment

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

we can remove this

.cursor/mcp.json Outdated
"Figma": {
"url": "https://mcp.figma.com/mcp",
"headers": {}
},
Copy link
Contributor

Choose a reason for hiding this comment

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

remove changes

@@ -0,0 +1,59 @@
import cors from 'cors';
Copy link
Contributor

Choose a reason for hiding this comment

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

This feature shouldn't require an application. Especially not an express server which we would have to set up infra for. The scope of changes for this feature should, in theory, be limited solely to apps/docs

@@ -0,0 +1,20 @@
---
id: color-pairing-tool
Copy link
Contributor

Choose a reason for hiding this comment

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

let's discuss a different place to organize the color picker. Getting Started doesn't fool like the right place

@@ -0,0 +1,188 @@
/* ── Color Picker ──────────────────────────────────────────────────── */
Copy link
Contributor

Choose a reason for hiding this comment

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

i dont think changes were meant to be made to vite-app. Likely the agent wen through multiple implementation attempts and forgot to clean up the changes made here. All the changes under vite-app should be removed

package.json Outdated
@@ -202,6 +193,7 @@
"postcss-sort-media-queries": "^4.2.1",
"postcss-styled-syntax": "^0.7.1",
"prettier": "^3.6.2",
"puppeteer": "^24.37.5",
Copy link
Contributor

Choose a reason for hiding this comment

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

remove new dependency

@@ -0,0 +1 @@
https://www.nerdwallet.com/banking/best/high-yield-online-savings-accounts
Copy link
Contributor

Choose a reason for hiding this comment

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

remove

@@ -260,20 +260,19 @@ export async function docgenRunner(params: DocgenRunnerParams): Promise<PluginCo
template: 'shared/objectMap',
});

/** Styles API data - extracted from *ClassNames exports */
Copy link
Contributor

Choose a reason for hiding this comment

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

im skeptical these docgen changes impact anything - we should remove all the changes under that folder

@@ -0,0 +1,125 @@
# Setup Guide
Copy link
Contributor

Choose a reason for hiding this comment

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

remove all guidelines

Copy link
Contributor

@hcopp hcopp left a comment

Choose a reason for hiding this comment

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

Love the idea! Hopefully this can help you talk with Cursor about how to clean things up.

Also if you could get sample screenshots, description of your intended changes, and a link to the figma in the PR description that would be very helpful!

@@ -74,9 +54,6 @@ export const MetadataLinks = memo(({ source, storybook, changelog, figma }: Meta
</LinkChip>
</Tooltip>
)}
<Chip compact onClick={handleCopyLLMDoc} start={<Icon color="fg" name="copy" size="s" />}>
Copy for LLM
</Chip>
Copy link
Contributor

Choose a reason for hiding this comment

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

We shouldn't remove this.

Copy link
Contributor

Choose a reason for hiding this comment

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

nit: these are normally svg, can we do that?

import { Icon } from '@coinbase/cds-web/icons';
import { TextInput } from '@coinbase/cds-web/controls';
import { toHex, hsbToRgb, rgbToHsb, parseColorInput } from './colorUtils';
import styles from './ColorPicker.module.css';
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you run yarn nx run docs:lint --fix

</Box>
<LineChart
enableScrubbing
height={{ base: 110 }}
Copy link
Contributor

Choose a reason for hiding this comment

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

You can just do height={110}

alt=""
src={imgSrc || '/img/checker-placeholder.png'}
style={{ width: '100%', height: '100%', objectFit: 'cover' }}
/>
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need this checker placeholder png? Maybe we could make it a simple svg

width: '100%',
height: '100%',
flexGrow: 1,
}}
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we set these on Box instead of in styles props

padding: '1px 5px',
borderRadius: 3,
whiteSpace: 'nowrap',
};
Copy link
Contributor

Choose a reason for hiding this comment

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

Ideally we don't need custom CSS here, but if anything we should be consistent with a separate module.css file as we have elsewhere. We can do <Text background={X} borderRadius={100} ... /> instead.

CDS layout components use inline styles generated from their style props,
which have higher specificity than CSS class rules. !important is required
here to override those inline styles at mobile breakpoints.
────────────────────────────────────────────────────────────────────────── */
Copy link
Contributor

Choose a reason for hiding this comment

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

We shouldn't need to use style props, but instead use our regular component props which support responsive values.

@@ -0,0 +1,3 @@
import type React from 'react';

export const monoStyle: React.CSSProperties = { fontFamily: 'CoinbaseMono, monospace' };
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this needed? We normally don't use mono font and even then don't have Coinbase fonts in our docs

default:
return state;
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

We normally don't use a state file, but I get it could be funky here. Ideally we have a parent component which holds the state, and then passes the state components as props to different children based on what step we are on.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Development

Successfully merging this pull request may close these issues.

4 participants