fix(color): the text color selection does not show a checkmark #184
Conversation
WalkthroughThe pull request involves significant updates to the Changes
Possibly related PRs
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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)
Other keywords and placeholders
CodeRabbit Configuration File (
|
WalkthroughThis pull request addresses a bug fix related to the text color selection in the toolbar, where the checkmark was not displaying correctly. The changes involve updating the color palette used in the toolbar's SCSS file. Changes
|
![petercat-assistant[bot]](https://avatars.githubusercontent.com/in/875736?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
packages/fluent-editor/src/assets/toolbar.scss (1)
Line range hint
315-368: Add checkmark styling for selected color itemsThe color picker items are missing the checkmark styling that's present in other picker types. This is why selected colors don't show a checkmark indicator.
Add the following styles to show checkmarks for selected colors:
.ql-picker-item { width: 20px; height: 20px; margin: 0 5px 5px 0; display: inline-flex; align-items: center; + justify-content: center; color: #fff; + &.ql-selected::after { + content: '\2713'; /* Unicode checkmark */ + font-size: 14px; + font-weight: bold; + } &:hover { color: #fff; }This change will:
- Center-align the content in color picker items
- Add a checkmark (✓) to selected colors
- Style the checkmark to be visible on any background color
🧹 Nitpick comments (1)
packages/fluent-editor/src/assets/toolbar.scss (1)
Line range hint
161-368: Summary of findings and recommendationsWhile the color palette update is valid, it doesn't address the core issue of missing checkmarks in the color selection UI. The actual fix requires adding proper styling for selected color items. I recommend:
- Keep the color palette changes
- Add the suggested checkmark styling
- Consider adding a test case to verify the visual indicator works
Would you like me to help create a test case for this UI component?
Signed-off-by: wangry <qwangry123@163.com>
qwangry
force-pushed
the
wangry/fix-color
branch
from
3206c80 to
4967e9c
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
packages/fluent-editor/src/assets/toolbar.scss (1)
Color and background pickers need checkmark styles for selected items
Based on the search results, I can confirm that while header pickers have proper checkmark styles using
::afterpseudo-elements, the color and background pickers lack these styles. Specifically:
- Header picker items have checkmark styles:
&.ql-header .ql-picker-item[data-value='1']::after { font-size: 2em; }- Color and background pickers only have basic hover styles:
.ql-picker-item:hover { color: #252b3a; background-color: #f2f5fc; }The selected state is managed through the
.ql-selectedclass, but there's no visual checkmark indicator for the color and background pickers to show which option is currently selected.🔗 Analysis chain
Line range hint
208-251: Verify checkmark implementation across pickersThe file shows checkmark styles for other pickers (header, font, lineheight) but not for the color picker. Let's verify this pattern across the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Compare checkmark implementations across different pickers # Search for checkmark styles in picker implementations echo "Searching for checkmark styles in pickers..." rg -A 5 "ql-picker.*::after" --type scss # Search for selected state styles echo "Searching for selected state styles..." rg -A 5 "ql-selected" --type scssLength of output: 320
Script:
#!/bin/bash # Description: Compare checkmark implementations across different pickers without file type filtering # Search for checkmark styles in picker implementations echo "Searching for checkmark styles in pickers..." rg -A 5 "ql-picker.*::after" # Search for selected state styles echo "Searching for selected state styles..." rg -A 5 "ql-selected" # Also search for general picker styles to understand the pattern echo "Searching for picker style patterns..." rg -A 5 "\.ql-picker-item"Length of output: 10307
🧹 Nitpick comments (1)
packages/fluent-editor/src/assets/toolbar.scss (1)
214-216: Unrelated changes to font sizesThe additions to
$pickerGroupare unrelated to the text color selection checkmark issue. Consider moving these font size changes to a separate PR to maintain focused changes.Also applies to: 234-234, 236-236, 241-241, 244-244
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/fluent-editor/src/assets/toolbar.scss(3 hunks)
🔇 Additional comments (1)
packages/fluent-editor/src/assets/toolbar.scss (1)
161-164:
Color palette update alone won't fix the checkmark issue
While the color values have been updated, this change doesn't address the core issue of missing checkmark styles for selected colors.
The color picker needs styles for the selected state, similar to other pickers. Add the following CSS:
.ql-toolbar {
.ql-picker.ql-expanded {
&.ql-color,
&.ql-background {
.ql-picker-item {
+ position: relative;
+
+ &.ql-selected::after {
+ content: '✓';
+ position: absolute;
+ right: 4px;
+ top: 50%;
+ transform: translateY(-50%);
+ color: white;
+ text-shadow: 0 0 2px rgba(0, 0, 0, 0.8);
+ }
+
+ &.ql-selected[data-value='#ffffff']::after,
+ &.ql-selected[data-value='#facccc']::after,
+ &.ql-selected[data-value='#ffebcc']::after,
+ &.ql-selected[data-value='#ffffcc']::after,
+ &.ql-selected[data-value='#cce8cc']::after,
+ &.ql-selected[data-value='#cce0f5']::after,
+ &.ql-selected[data-value='#ebd6ff']::after {
+ color: #252b3a;
+ text-shadow: none;
+ }
}
}
}
}Likely invalid or redundant comment.
|
测试发现label的选项有的也不显示对勾，然后发现有些信息比如字号大小是用户自定义配置的，这样在css中如何处理呢？遍历的话也太多了🤔 |
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
packages/fluent-editor/src/assets/toolbar.scss (1)
Line range hint
279-397: Fix missing checkmark in color selectionThe color picker lacks the checkmark styles that other pickers have. To fix this, add checkmark styles for selected color items:
.ql-color-picker { .ql-picker-item { width: 20px; height: 20px; margin: 0 5px 5px 0; display: inline-flex; align-items: center; color: #fff; + position: relative; /* Added for checkmark positioning */ + &.ql-selected::after { + content: '✓'; + position: absolute; + top: 50%; + left: 50%; + transform: translate(-50%, -50%); + font-size: 14px; + color: inherit; + } &:hover { color: #fff; }This solution:
- Adds relative positioning to color picker items
- Implements checkmark using
::afterpseudo-element for selected items- Centers the checkmark within the color box
- Inherits text color from parent for proper contrast
🧹 Nitpick comments (1)
packages/fluent-editor/src/assets/toolbar.scss (1)
214-216: Unrelated changes to picker optionsThese additions to
$pickerGroupexpand the header levels and font size options but are unrelated to the color selection checkmark issue. Consider moving these changes to a separate PR to maintain focused commits.Also applies to: 234-234, 236-236, 241-241, 244-244
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/fluent-editor/src/assets/toolbar.scss(3 hunks)
🔇 Additional comments (1)
packages/fluent-editor/src/assets/toolbar.scss (1)
161-164: Color palette update looks good, but won't fix the checkmark issue
While the color values are properly formatted, changing the color palette alone won't address the missing checkmark in the color selection UI.
The core issue lies in the missing checkmark styles for selected colors, not in the color values themselves.
PR
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
What is the current behavior?
Issue Number: #173
What is the new behavior?
Does this PR introduce a breaking change?
Other information
Summary by CodeRabbit