TINY-11619: fix input and size input css inside toolbar #10021
Conversation
ztomaszyk
requested review from
a team,
spocke,
lorenzo-pomili,
hamza0867 and
HAFRMO
and removed request for
a team
WalkthroughThe changes in this pull request introduce new CSS variables and rules for styling toolbar buttons and text fields within a context form group. Specifically, it adds styles for icon buttons, ensuring they have appropriate margin and padding, and aligns the height of text fields with toolbar buttons. These adjustments aim to enhance the consistency and maintainability of the styles across the components. Changes
Assessment against linked issues
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
🪧 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
modules/oxide/src/less/theme/components/button/button-icon.less (1)
19-19: Track this technical debt for future refactoringThe TODO comment indicates that this is a temporary solution mixing dialog and toolbar components. This should be tracked to ensure the size input is properly refactored into a separate component.
Would you like me to create a GitHub issue to track this technical debt and the planned refactoring work?
modules/oxide/src/less/theme/components/toolbar-button/toolbar-button.less (1)
22-23: Document the purpose of the +1px margin adjustment.While extracting these values into variables improves maintainability, please add a comment explaining why we need the additional 1px in
@toolbar-button-margin. This will help future maintainers understand the reasoning behind this specific adjustment.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
modules/oxide/src/less/theme/components/button/button-icon.less(1 hunks)modules/oxide/src/less/theme/components/form/textfield.less(2 hunks)modules/oxide/src/less/theme/components/toolbar-button/toolbar-button.less(2 hunks)
🔇 Additional comments (4)
modules/oxide/src/less/theme/components/button/button-icon.less (1)
20-26: LGTM! Verify toolbar variables are properly defined
The CSS implementation looks good and aligns with the PR objectives. The use of @toolbar-button-margin and @toolbar-button-padding variables promotes consistency.
Let's verify these variables are properly defined:
✅ Verification successful
Variables are properly defined in toolbar-button.less
Both variables @toolbar-button-margin and @toolbar-button-padding are defined in modules/oxide/src/less/theme/components/toolbar-button/toolbar-button.less:
@toolbar-button-margin: (@toolbar-button-spacing-y + 1px) @toolbar-button-spacing-x @toolbar-button-spacing-y 0@toolbar-button-padding: 0
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if the toolbar button variables are defined in the expected locations
# Expected: Find definitions of @toolbar-button-margin and @toolbar-button-padding
# Search for toolbar button variable definitions
rg -t less "@toolbar-button-(margin|padding)\s*:" modules/oxide/src/less/
Length of output: 366
modules/oxide/src/less/theme/components/form/textfield.less (2)
28-29: LGTM! Good variable naming and reuse.
The new variable promotes consistency by reusing the toolbar button height for text fields.
69-70: LGTM! Verify impact on other skins.
The changes correctly address the height issues by removing the minimum height constraint and setting an explicit height. This should fix the 'small' skin issues mentioned in TINY-11619.
Let's verify if these changes affect other skins:
✅ Verification successful
Let me gather more information about the skins and theme structure to ensure a thorough verification.
Let me check the specific skin implementations to ensure there are no overrides.
Changes are safe across all skins and themes
The toolbar textfield height changes are safe because:
- The height is derived from
@toolbar-button-heightwhich is properly defined in both the default theme and skins - The
tinymce-5skin defines it as34pxwhile the default theme sets it to28px - No skin overrides the textfield-specific styles or the height variable
- The removal of
min-heightdoesn't conflict with any skin-specific styles
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for other skin-specific styles that might conflict with these changes
# Search for skin-specific styles affecting toolbar text fields
rg -l "tox-toolbar-textfield" modules/oxide/src/less/theme/skins/
# Search for other height-related styles for toolbar text fields
rg "height.*toolbar.*textfield|toolbar.*textfield.*height" modules/oxide/src/less/theme/
Length of output: 442
Script:
#!/bin/bash
# Let's check the overall theme and skins structure
fd -t d "skins|theme" modules/oxide/src
# Check for any textfield-related styles across the codebase
rg -l "tox-toolbar-textfield" modules/oxide/src
# Check the variable definition and its usage
rg "@toolbar-button-height" modules/oxide/src
Length of output: 1418
Script:
#!/bin/bash
# Check skin-specific styles
rg -l "textfield|toolbar" modules/oxide/src/less/skins/
# Get content of any skin files that might affect toolbar or textfield styles
fd . modules/oxide/src/less/skins/ -e less --exec cat {}
Length of output: 22628
modules/oxide/src/less/theme/components/toolbar-button/toolbar-button.less (1)
72-72: Verify the changes with the 'small' skin.
The changes look good and improve maintainability. However, since the PR's main objective is to fix input styling with the 'small' skin, we should verify that these changes achieve the desired effect.
Let's verify if there are any skin-specific overrides that might affect these changes:
Also applies to: 74-74
✅ Verification successful
The margin changes are safe and do not affect the 'small' skin
After analyzing the toolbar-button.less file, I can confirm that:
- There are no skin-specific styles that override these values
- The margin is controlled by variables that are independent of skin types
- The only size variations are handled through specific classes (.tox-tbtn--md and .tox-tbtn--lg) which don't affect the base margin
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any skin-specific styles that might override these values
rg -l "small.*skin|skin.*small" --type less
Length of output: 45
Script:
#!/bin/bash
# Let's try a broader search for skin-related styles and toolbar button styles
rg -g "*.less" "\.tox-tbtn.*skin|skin.*\.tox-tbtn" -l
# Also search for any small-related styles in toolbar button
rg -g "*.less" "\.tox-tbtn.*small|small.*\.tox-tbtn" -l
# Let's also check the specific file for any skin or size-related modifiers
cat "modules/oxide/src/less/theme/components/toolbar-button/toolbar-button.less"
Length of output: 8403
ztomaszyk
force-pushed
the
feature/TINY-11619
branch
from
6bcca03 to
8d3f08b
Compare
Related Ticket: TINY-11619
Description of Changes:
Fix input and size input css inside toolbar for skin: 'small'
Pre-checks:
Changelog entry addedTests have been added (if applicable)feature/,hotfix/orspike/Review:
GitHub issues (if applicable):
Summary by CodeRabbit
New Features
Chores