feat: social sources redesign #750
Conversation
WalkthroughThe pull request introduces significant changes across multiple files, focusing on enhancing functionality, improving error handling, and refining styles for various components. Key modifications include updates to the Changes
Poem
🪧 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 (
|
egordidenko
force-pushed
the
feat/new-social-sources
branch
from
452d20d to
c4ae71d
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Outside diff range and nitpick comments (10)
blocks/ExternalSource/ExternalSource.js (1)
106-107: Remove unused variablevalin subscription callbackThe variable
valis defined but never used in thesubConfigValue('localeName', ...)callback. To clean up the code, consider removing the unused parameter.Apply this diff to remove the unused parameter:
-this.subConfigValue('localeName', (val) => { +this.subConfigValue('localeName', () => { this.setupL10n(); });🧰 Tools
🪛 GitHub Check: build
[warning] 106-106:
'val' is defined but never usedblocks/ExternalSource/buildThemeDefinition.js (1)
7-10: Consider caching computed stylesThe
getCssValuefunction creates a newCSSStyleDeclarationobject on each call. For better performance when processing multiple properties, consider caching the computed style.-function getCssValue(element, propName) { - let style = window.getComputedStyle(element); - return style.getPropertyValue(propName).trim(); +function getCssValue(element, propName, cachedStyle) { + const style = cachedStyle || window.getComputedStyle(element); + return style.getPropertyValue(propName).trim(); }blocks/ExternalSource/external-source.css (1)
48-65: Consider adding transition for hover effectThe button hover underline effect could benefit from a smooth transition.
uc-external-source .uc-selection-status-box button { color: var(--uc-primary); height: auto; padding: 0; background: none; + transition: text-decoration var(--uc-transition); }blocks/CameraSource/camera-source.css (1)
43-43: Consider adding min-height constraintSetting
height: 100%ensures content fills the container, but consider adding amin-heightto prevent content compression in edge cases..uc-content { height: 100%; + min-height: 300px; }demo/new-social-sources-test.html (2)
5-5: Add complete viewport meta attributesThe current viewport meta tag might cause accessibility issues. Consider adding more comprehensive viewport settings.
- <meta name="viewport" content="width=device-width, initial-scale=1.0, maximum-scale=1.0, user-scalable=0" /> + <meta name="viewport" content="width=device-width, initial-scale=1.0, minimum-scale=1.0" />
41-48: Consider persisting theme preferenceThe theme switching logic could be improved by persisting user preference.
darkMode.addEventListener('change', () => { + const preference = darkMode.checked ? 'dark' : 'light'; + localStorage.setItem('theme-preference', preference); const uploader = document.querySelector('uc-file-uploader-regular'); uploader.classList.toggle('uc-dark', darkMode.checked); uploader.classList.toggle('uc-light', !darkMode.checked); document.body.classList.toggle('dark', darkMode.checked); document.body.classList.toggle('light', !darkMode.checked); }); + // Initialize theme from stored preference + const storedPreference = localStorage.getItem('theme-preference'); + if (storedPreference) { + darkMode.checked = storedPreference === 'dark'; + darkMode.dispatchEvent(new Event('change')); + }locales/file-uploader/en.js (1)
29-31: LGTM! Consider adding pluralization support.The new localization strings improve user experience by providing clear feedback on file selection status and actions. However, consider adding pluralization support for the count message to handle singular/plural cases correctly.
- 'selected-count': '{{count}} of {{total}} selected', + 'selected-count': '{{count}} {{plural:file(count)}} of {{total}} selected',blocks/ExternalSource/types.js (1)
28-52: Consider adding validation for selection counts.The InputMessageMap type effectively handles different selection states, but consider adding runtime validation to ensure selectedCount never exceeds total.
type ValidatedSelectionMessage = { type: 'selected-files-change'; total: number; selectedCount: number & { __brand: 'ValidatedCount' }; // Branded type for compile-time safety };blocks/themes/uc-basic/theme.css (1)
279-279: Fix missing newline.Add a newline at the end of the file to follow CSS best practices.
blocks/Modal/modal.css (1)
27-30: LGTM! Consider extracting common calculations into CSS custom properties.The responsive sizing logic is well-implemented using the
min()function and design system variables. The calculations ensure the modal maintains proper spacing and constraints while being responsive.Consider extracting the repeated padding calculation into a CSS custom property for better maintainability:
:where([uc-modal]) > dialog { + --modal-spacing: calc(100% - var(--uc-padding) * 2); width: min(var(--uc-dialog-width), 100%); - max-width: min(calc(100% - var(--uc-padding) * 2), var(--uc-dialog-max-width)); + max-width: min(var(--modal-spacing), var(--uc-dialog-max-width)); min-height: var(--uc-button-size); - max-height: min(calc(100% - var(--uc-padding) * 2), var(--uc-dialog-max-height)); + max-height: min(var(--modal-spacing), var(--uc-dialog-max-height));
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (18)
blocks/CameraSource/CameraSource.js(0 hunks)blocks/CameraSource/camera-source.css(2 hunks)blocks/CloudImageEditorActivity/index.css(1 hunks)blocks/ExternalSource/ExternalSource.js(6 hunks)blocks/ExternalSource/MessageBridge.js(1 hunks)blocks/ExternalSource/buildStyles.js(0 hunks)blocks/ExternalSource/buildThemeDefinition.js(1 hunks)blocks/ExternalSource/external-source.css(3 hunks)blocks/ExternalSource/messages.js(0 hunks)blocks/ExternalSource/types.js(1 hunks)blocks/Modal/Modal.js(1 hunks)blocks/Modal/modal.css(1 hunks)blocks/StartFrom/start-from.css(1 hunks)blocks/UploadList/upload-list.css(1 hunks)blocks/UrlSource/url-source.css(0 hunks)blocks/themes/uc-basic/theme.css(2 hunks)demo/new-social-sources-test.html(1 hunks)locales/file-uploader/en.js(1 hunks)
💤 Files with no reviewable changes (4)
- blocks/UrlSource/url-source.css
- blocks/ExternalSource/buildStyles.js
- blocks/ExternalSource/messages.js
- blocks/CameraSource/CameraSource.js
🧰 Additional context used
🪛 GitHub Check: build
blocks/ExternalSource/ExternalSource.js
[warning] 106-106:
'val' is defined but never used
🔇 Additional comments (10)
blocks/ExternalSource/MessageBridge.js (1)
65-65: Specify the target origin in postMessage for security
Using '*' as the target origin in postMessage can pose security risks, as it allows messages to be sent to any window with any origin. It's recommended to specify the expected origin to ensure messages are only sent to trusted domains.
[security_issue]
Consider updating the postMessage call to specify the target origin:
this._context.postMessage(message, '*');
+// Replace '*' with the specific origin, e.g., this._context.originblocks/StartFrom/start-from.css (1)
16-17: Potential browser compatibility issues with the :has() pseudo-class
Using the :has() CSS pseudo-class may cause compatibility problems in browsers that do not support it, affecting the layout and functionality of the modal. Please verify browser support and consider adding fallbacks where necessary.
Run the following script to identify all instances of :has() in CSS files:
blocks/UploadList/upload-list.css (1)
5-5: Verify layout behavior with height: max-content
The change from height: 100% to height: max-content could affect the component's layout behavior, especially in containers that expect the upload list to fill their height.
Let's check for potential layout issues:
✅ Verification successful
Let me gather more context about the usage of this component to understand the layout implications better.
Height change to max-content is appropriate
The change from height: 100% to height: max-content is actually beneficial for the component's layout:
-
The component uses a flex column layout with proper flex distribution:
- Root element is
display: flexwithflex-direction: column .uc-fileshasflex: 1to take remaining space- Child elements have explicit heights or min-heights where needed
- Root element is
-
The
max-contentheight allows the component to:- Grow naturally based on its content
- Maintain scrollability through
.uc-filesoverflow - Work correctly in both modal and non-modal contexts
The layout structure ensures proper content distribution regardless of container height.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for parent containers that might rely on upload-list's height
rg -l "uc-upload-list" --type css | xargs rg "height|flex|grid"
Length of output: 1318
Script:
#!/bin/bash
# Search for HTML/JSX files using the upload-list component
fd -e html -e jsx -e tsx | xargs rg -l "uc-upload-list"
# Get the full content of upload-list.css to understand the complete styling context
cat blocks/UploadList/upload-list.css
Length of output: 1425
blocks/ExternalSource/external-source.css (3)
35-42: LGTM: Simplified toolbar layout
The change from grid to flex layout with space-between is a good simplification that maintains the same visual structure.
11-13: Check browser compatibility for :has() selector
The :has() selector is relatively new and might not be supported in all browsers. Consider adding a fallback for better browser compatibility.
Let's check browser usage statistics:
✅ Verification successful
:has() selector usage is consistent with modern browser support patterns
The codebase already demonstrates a pattern of using modern CSS features with proper feature detection through @supports. The :has() selector is consistently used across multiple modal dialogs and other components, indicating an intentional architectural decision. Given that:
- The
:has()selector has broad modern browser support (>95% globally) - The codebase shows awareness of compatibility with
@supportschecks where needed - The selector is used consistently in similar modal dialog patterns
No fallback is necessary as this appears to be an intentional choice aligned with the project's browser support requirements.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other uses of :has() selector
rg ":has\(" --type css
Length of output: 801
Script:
#!/bin/bash
# Let's check if there are any browser compatibility comments or fallbacks in the codebase
rg -i "browser|compat|fallback|support" --type css -C 2
Length of output: 1309
67-79: Verify z-index stacking context
The absolute positioning with z-index might create stacking context issues. Ensure the header remains above other content.
Let's check for potential z-index conflicts:
✅ Verification successful
Let me gather more context about the ExternalSource component's structure and its relationship with other elements.
Based on the gathered information, I can now generate the final response:
Z-index implementation is appropriate for this context
The z-index:1 for the activity header is sufficient because:
- The component is rendered within a modal dialog context
- The header only needs to be above its sibling content (iframe and toolbar)
- Other high z-index values in the codebase (like DropArea's 2147483647) are for components that operate at a different stacking context
- The pointer-events handling ensures proper interaction despite the absolute positioning
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other z-index declarations
rg "z-index:" --type css
Length of output: 1005
Script:
#!/bin/bash
# Check the HTML structure and related CSS for ExternalSource
rg -A 5 "uc-external-source" --type html --type css
Length of output: 4000
blocks/CameraSource/camera-source.css (1)
13-15: Verify modal behavior on different viewport sizes
The new selector forces the dialog to occupy the full viewport. While this works for camera functionality, ensure it doesn't cause issues with:
- Very tall viewports where camera feed might get stretched
- Mobile devices in landscape orientation
blocks/ExternalSource/types.js (2)
3-14: LGTM! Well-structured Instagram type definition.
The InstagramInfo type properly captures all necessary metadata for Instagram integration with appropriate optional fields.
62-120: LGTM! Comprehensive theme definition.
The ThemeDefinition type provides excellent coverage of all theme variables with clear naming conventions.
blocks/themes/uc-basic/theme.css (1)
16-17: Verify dialog dimensions for different screen sizes.
The increased dialog dimensions might improve content visibility, but please verify:
- Content remains accessible on smaller screens
- No horizontal scrollbars appear on common viewport sizes
- Dialog positioning remains centered
✅ Verification successful
Let's gather more information about the responsive styles in these files.
Dialog dimensions are properly handled with responsive breakpoints
The code review reveals that the dialog dimensions are well-managed with appropriate media queries:
-
For screens below 600px height:
@media only screen and (max-height: 600px) { --uc-dialog-max-height: 100%; }
-
For narrow screens below 430px width:
@media only screen and (max-width: 430px) { --uc-dialog-max-width: 100vw; --uc-dialog-max-height: var(--uploadcare-blocks-window-height); }
The modal implementation also ensures proper spacing with:
- Padding consideration:
max-width: min(calc(100% - var(--uc-padding) * 2), var(--uc-dialog-max-width)) - Flexible width:
width: min(var(--uc-dialog-width), 100%) - Proper centering:
margin: auto
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any related responsive layout styles or dimension overrides
rg -l "dialog-max-width|dialog-max-height" --type css
Length of output: 111
Script:
#!/bin/bash
# Check the content of both files to analyze responsive styles
echo "=== blocks/themes/uc-basic/theme.css ==="
cat blocks/themes/uc-basic/theme.css
echo -e "\n=== blocks/Modal/modal.css ==="
cat blocks/Modal/modal.css
Length of output: 12750
| 'type' in message && | ||
| MESSAGE_TYPE_WHITELIST.includes(/** @type {import('./types').InputMessageType} */ (message.type)) | ||
| ); |
There was a problem hiding this comment.
Ensure message.type is a string before using it
To prevent potential runtime errors, it's important to verify that message.type is a string before using it with includes(). This ensures the method functions correctly even if message.type is not a string.
Apply this diff to add the type check:
return (
'type' in message &&
+ typeof message.type === 'string' &&
MESSAGE_TYPE_WHITELIST.includes(/** @type {import('./types').InputMessageType} */ (message.type))
);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| 'type' in message && | |
| MESSAGE_TYPE_WHITELIST.includes(/** @type {import('./types').InputMessageType} */ (message.type)) | |
| ); | |
| 'type' in message && | |
| typeof message.type === 'string' && | |
| MESSAGE_TYPE_WHITELIST.includes(/** @type {import('./types').InputMessageType} */ (message.type)) | |
| ); |
| [uc-modal] > dialog:has(uc-cloud-image-editor-activity[active]) { | ||
| width: 100%; | ||
| height: 100%; |
There was a problem hiding this comment.
💡 Codebase verification
:has() selector usage needs fallback styles
The codebase search reveals that while there are default dialog styles in blocks/Modal/modal.css using :where([uc-modal]) > dialog, these don't fully address the width/height requirements when specific components are active. The :has() selector is used to apply full-width/height styles for certain components, but there's no fallback mechanism for browsers that don't support it.
blocks/Modal/modal.cssprovides base dialog styles with fixed widths- Components like cloud-image-editor, camera-source, and external-source require full-width/height when active
- No alternative styling mechanism exists for browsers without
:has()support
Recommendation: Add fallback styles using a parent class or attribute that can be toggled via JavaScript when these components are active. For example:
[uc-modal][full-size] > dialog,
[uc-modal] > dialog:has(uc-cloud-image-editor-activity[active]) {
width: 100%;
height: 100%;
}🔗 Analysis chain
Potential browser compatibility issues with the :has() pseudo-class
The :has() CSS pseudo-class is not fully supported in all browsers, which may lead to layout issues for some users. Ensure that the target browsers support :has(), or provide a fallback for those that do not.
Run the following script to find all usages of :has() and verify browser compatibility:
:has() selector usage requires fallback styles for older browsers
Based on the search results, the :has() selector is used consistently across multiple CSS files in modal dialogs and file uploader components. This pattern affects core UI functionality and needs fallback styles to ensure compatibility with older browsers. Here are the affected files:
blocks/CloudImageEditorActivity/index.css:10blocks/StartFrom/start-from.css:16blocks/ExternalSource/external-source.css:11blocks/CameraSource/camera-source.css:13solutions/file-uploader/inline/index.css(multiple instances)
Let's verify if there are any existing fallback styles:
Let me try one more search to check for any default dialog styles or JavaScript-based fallbacks:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for all CSS files using ':has()' and list them.
# Find all CSS files containing ':has('
fd -e css -x grep -Hn ':has(' {}
# Check browser compatibility for ':has()' pseudo-class.
echo "Refer to https://caniuse.com/css-has for browser support details."
Length of output: 989
Script:
#!/bin/bash
# Search for alternative styling approaches for these components
# that might serve as fallbacks
# Check for dialog styling without :has()
rg -g "*.css" "dialog\s*{|dialog\[|dialog\." -A 2
# Check for component-specific styling without :has()
rg -g "*.css" "uc-cloud-image-editor-activity\[active\]|uc-start-from\[active\]|uc-external-source\[active\]|uc-camera-source\[active\]" -A 2
Length of output: 1136
Script:
#!/bin/bash
# Search for default dialog styles and potential JS-based solutions
rg -g "*.css" "dialog[^:]" -A 2
# Search for dialog-related JavaScript that might handle fallbacks
rg -g "*.{js,ts}" "dialog.*active" -A 2
Length of output: 4628
| /** @param {HTMLElement} element */ | ||
| export function buildThemeDefinition(element) { | ||
| return ucCustomProperties.reduce((acc, prop) => { | ||
| const value = getCssValue(element, prop); | ||
| if (value) { | ||
| acc[prop] = value; | ||
| } | ||
| return acc; | ||
| }, /** @type {Record<keyof import('./types.js').ThemeDefinition, string>} */ ({})); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding validation for required theme properties
The current implementation silently skips undefined values. Consider validating essential theme properties to ensure a complete theme definition.
export function buildThemeDefinition(element) {
+ const requiredProps = ['--uc-background', '--uc-foreground', '--uc-primary'];
const theme = ucCustomProperties.reduce((acc, prop) => {
const value = getCssValue(element, prop);
if (value) {
acc[prop] = value;
}
return acc;
}, /** @type {Record<keyof import('./types.js').ThemeDefinition, string>} */ ({}));
+ const missingProps = requiredProps.filter(prop => !theme[prop]);
+ if (missingProps.length > 0) {
+ throw new Error(`Missing required theme properties: ${missingProps.join(', ')}`);
+ }
+ return theme;
}Committable suggestion skipped: line range outside the PR's diff.
|
|
||
| this.ref.dialog.addEventListener('close', this._handleDialogClose); | ||
| this.ref.dialog.addEventListener('mousedown', this._handleDialogMouseDown); | ||
| this.ref.dialog.addEventListener('mouseup', this._handleDialogMouseUp); |
There was a problem hiding this comment.
Add browser compatibility check for dialog element
The unconditional addition of event listeners assumes HTMLDialogElement support. This might cause issues in older browsers or certain mobile browsers.
initCallback() {
super.initCallback();
+ if (!('showModal' in HTMLDialogElement.prototype)) {
+ console.warn('Dialog element not fully supported in this browser');
+ return;
+ }
this.ref.dialog.addEventListener('close', this._handleDialogClose);
this.ref.dialog.addEventListener('mousedown', this._handleDialogMouseDown);
this.ref.dialog.addEventListener('mouseup', this._handleDialogMouseUp);Committable suggestion skipped: line range outside the PR's diff.
demo/new-social-sources-test.html
Outdated
| pubkey="8ae495b8b715101b5119" | ||
| social-base-url="https://neutral-jolly-heron.ngrok-free.app" |
There was a problem hiding this comment.
Remove sensitive information from demo file
The file contains potentially sensitive information:
- Public key should be loaded from environment variables
- ngrok URL should be replaced with a placeholder or configuration variable
- pubkey="8ae495b8b715101b5119"
- social-base-url="https://neutral-jolly-heron.ngrok-free.app"
+ pubkey="${UPLOADCARE_PUBLIC_KEY}"
+ social-base-url="${SOCIAL_BASE_URL}"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| pubkey="8ae495b8b715101b5119" | |
| social-base-url="https://neutral-jolly-heron.ngrok-free.app" | |
| pubkey="${UPLOADCARE_PUBLIC_KEY}" | |
| social-base-url="${SOCIAL_BASE_URL}" |
| <fieldset> | ||
| <legend>Options</legend> | ||
| <label> | ||
| <input id="dark-mode" type="checkbox" /> | ||
| Dark Mode | ||
| </label> | ||
| <label> | ||
| <input id="multiple" type="checkbox" checked /> | ||
| Multiple | ||
| </label> | ||
| <label> | ||
| <input id="images-only" type="checkbox" /> | ||
| Images Only | ||
| </label> | ||
| </fieldset> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance accessibility of form controls
The fieldset controls lack proper accessibility attributes.
<fieldset>
<legend>Options</legend>
- <label>
+ <label role="checkbox" aria-checked="false">
<input id="dark-mode" type="checkbox" />
Dark Mode
</label>
- <label>
+ <label role="checkbox" aria-checked="true">
<input id="multiple" type="checkbox" checked />
Multiple
</label>
- <label>
+ <label role="checkbox" aria-checked="false">
<input id="images-only" type="checkbox" />
Images Only
</label>
</fieldset>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <fieldset> | |
| <legend>Options</legend> | |
| <label> | |
| <input id="dark-mode" type="checkbox" /> | |
| Dark Mode | |
| </label> | |
| <label> | |
| <input id="multiple" type="checkbox" checked /> | |
| Multiple | |
| </label> | |
| <label> | |
| <input id="images-only" type="checkbox" /> | |
| Images Only | |
| </label> | |
| </fieldset> | |
| <fieldset> | |
| <legend>Options</legend> | |
| <label role="checkbox" aria-checked="false"> | |
| <input id="dark-mode" type="checkbox" /> | |
| Dark Mode | |
| </label> | |
| <label role="checkbox" aria-checked="true"> | |
| <input id="multiple" type="checkbox" checked /> | |
| Multiple | |
| </label> | |
| <label role="checkbox" aria-checked="false"> | |
| <input id="images-only" type="checkbox" /> | |
| Images Only | |
| </label> | |
| </fieldset> |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
test-locales.js (1)
61-65: Consider adding language code format validationWhile the simplified validation is good, consider adding a check for valid language code format (e.g., using BCP 47 language tag validation) to prevent invalid values in 'social-source-lang'.
Example implementation:
if (!socialSourceLang) { console.error( `Missing social-source-lang for locale "${localeName}" in group "${group}" (social-source-lang is required for social sources)`, ); anyError = true; + } else { + // Validate language code format using Intl.Locale + try { + new Intl.Locale(socialSourceLang); + } catch (err) { + console.error( + `Invalid social-source-lang format "${socialSourceLang}" for locale "${localeName}" in group "${group}"`, + ); + anyError = true; + } }locales/file-uploader/sv.js (1)
3-3: Language code update looks goodThe change from 'en' to 'sv' correctly aligns the social source language with the Swedish locale.
Consider implementing a validation system to ensure that
social-source-langalways matcheslocale-idin all locale files. This could prevent potential mismatches during future updates.Example validation in your build process:
function validateLocaleFile(content) { const localeId = content['locale-id']; const socialSourceLang = content['social-source-lang']; if (localeId !== socialSourceLang) { throw new Error(`Mismatch: locale-id (${localeId}) != social-source-lang (${socialSourceLang})`); } }locales/file-uploader/pl.js (1)
3-3: LGTM! Social source language correctly set to Polish.The change from 'en' to 'pl' properly aligns with the Polish locale identifier. The change is consistent with the pattern observed across other locale files.
These changes collectively improve the localization architecture by ensuring that social source interactions use the appropriate language for each locale, which enhances the user experience for non-English speakers.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (25)
demo/new-social-sources-test.html(1 hunks)locales/file-uploader/ar.js(1 hunks)locales/file-uploader/az.js(1 hunks)locales/file-uploader/ca.js(1 hunks)locales/file-uploader/cs.js(1 hunks)locales/file-uploader/da.js(1 hunks)locales/file-uploader/el.js(1 hunks)locales/file-uploader/et.js(1 hunks)locales/file-uploader/hy.js(1 hunks)locales/file-uploader/is.js(1 hunks)locales/file-uploader/ja.js(1 hunks)locales/file-uploader/ka.js(1 hunks)locales/file-uploader/kk.js(1 hunks)locales/file-uploader/ko.js(1 hunks)locales/file-uploader/lv.js(1 hunks)locales/file-uploader/nb.js(1 hunks)locales/file-uploader/nl.js(1 hunks)locales/file-uploader/pl.js(1 hunks)locales/file-uploader/pt.js(1 hunks)locales/file-uploader/ro.js(1 hunks)locales/file-uploader/sk.js(1 hunks)locales/file-uploader/sr.js(1 hunks)locales/file-uploader/sv.js(1 hunks)locales/file-uploader/vi.js(1 hunks)test-locales.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- demo/new-social-sources-test.html
🔇 Additional comments (23)
locales/file-uploader/ko.js (1)
3-3: LGTM: Appropriate language code update
The change from 'en' to 'ko' for 'social-source-lang' correctly reflects the Korean language context for social sources.
locales/file-uploader/ja.js (1)
3-3: LGTM: Appropriate language code update
The change from 'en' to 'ja' for 'social-source-lang' correctly reflects the Japanese language context for social sources.
locales/file-uploader/ar.js (1)
3-3: Language code update looks good
The change from 'en' to 'ar' correctly aligns the social source language with the Arabic locale.
Let's verify the consistency of this change across all locale files:
✅ Verification successful
Language code consistency verified across all locale files
The verification shows that all locale files in the locales/file-uploader/ directory have matching locale-id and social-source-lang values, confirming that the change in the Arabic locale file is consistent with the overall pattern used throughout the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that social-source-lang matches locale-id in all locale files
# Expected: Each file should have matching locale-id and social-source-lang
echo "Checking locale files consistency..."
for file in $(fd -t f . locales/file-uploader/); do
echo "Checking $file"
# Extract locale-id and social-source-lang values
rg -U "(?:'locale-id':\s*'([^']+)'|'social-source-lang':\s*'([^']+)')" "$file"
done
Length of output: 10843
locales/file-uploader/da.js (1)
3-3: Language code update looks good
The change from 'en' to 'da' correctly aligns the social source language with the Danish locale.
locales/file-uploader/nb.js (1)
3-3: LGTM! Correct locale alignment
The change from 'en' to 'nb' properly aligns the social source language with the Norwegian locale, improving consistency for Norwegian users.
locales/file-uploader/vi.js (1)
3-3: LGTM! Correct locale alignment
The change from 'en' to 'vi' properly aligns the social source language with the Vietnamese locale, improving consistency for Vietnamese users.
locales/file-uploader/et.js (1)
3-3: LGTM! Correct locale alignment
The change from 'en' to 'et' properly aligns the social source language with the Estonian locale, improving consistency for Estonian users.
Let's verify that this pattern is consistent across all locale files:
✅ Verification successful
Perfect consistency across all locale files
The verification confirms that all locale files in the file-uploader directory maintain perfect consistency between their 'locale-id' and 'social-source-lang' values. The Estonian locale change aligns with this established pattern.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that social-source-lang matches locale-id in all locale files
# Expected: Each locale file should have matching locale-id and social-source-lang values
echo "Checking locale files for consistency..."
for file in $(fd -e js . locales/file-uploader/); do
echo "Checking $file:"
# Extract and compare locale-id and social-source-lang
grep -A 1 "'locale-id':\|'social-source-lang':" "$file"
done
Length of output: 10700
locales/file-uploader/is.js (1)
3-3: LGTM! Language code correctly updated
The change from 'en' to 'is' properly aligns the social source language with the Icelandic locale identifier.
locales/file-uploader/az.js (1)
3-3: LGTM! Language code correctly updated
The change from 'en' to 'az' properly aligns the social source language with the Azerbaijani locale identifier.
locales/file-uploader/kk.js (2)
3-3: LGTM! Language code correctly updated
The change from 'en' to 'kk' properly aligns the social source language with the Kazakh locale identifier.
1-3: Verify consistency across all locale files
Let's ensure this change has been applied consistently across all locale files in the project.
✅ Verification successful
All locale files have consistent social-source-lang values
The verification shows that in all locale files under locales/file-uploader/, the social-source-lang value matches the corresponding locale-id. This includes the file in question (kk.js) as well as all other locale files. The consistency is maintained across all language variants.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if all locale files have their social-source-lang matching their locale-id
echo "Checking consistency of social-source-lang across locale files..."
for file in $(fd -e js . locales/file-uploader/); do
echo "Checking $file:"
# Extract and compare locale-id and social-source-lang
grep -A 1 "'locale-id':" "$file"
grep "'social-source-lang':" "$file"
echo "---"
done
Length of output: 12370
locales/file-uploader/hy.js (1)
3-3: LGTM! Social source language correctly set to Armenian.
The change from 'en' to 'hy' properly aligns with the Armenian locale identifier.
✅ Verification successful
Social source language is correctly set and consistent across all locale files
The verification confirms that each locale file in the file-uploader directory correctly sets its 'social-source-lang' value to match its respective locale identifier. The Armenian (hy) locale follows this same pattern, maintaining consistency with all other locale files.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify consistency of social-source-lang across locale files
rg -g 'locales/file-uploader/*.js' "'social-source-lang':" -A 1
Length of output: 4243
locales/file-uploader/sr.js (1)
3-3: LGTM! Social source language correctly set to Serbian.
The change from 'en' to 'sr' properly aligns with the Serbian locale identifier.
locales/file-uploader/ka.js (1)
3-3: LGTM! Verify completeness of locale updates
The change from 'en' to 'ka' for Georgian locale is correct.
Let's verify if all locale files have been updated consistently:
✅ Verification successful
Locale settings are consistent across all files
All locale files in the locales/file-uploader/ directory have their social-source-lang correctly set to match their respective language codes. The Georgian (ka) locale change aligns with this pattern, where each locale file uses its own ISO language code.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check all locale files for social-source-lang setting
rg -U "social-source-lang':\s*'([^']+)" locales/file-uploader/
Length of output: 2051
locales/file-uploader/sk.js (1)
3-3: LGTM! Language code is correct
The change from 'en' to 'sk' for Slovak locale is correct.
locales/file-uploader/cs.js (1)
3-3: LGTM! Language code is correct
The change from 'en' to 'cs' for Czech locale is correct.
locales/file-uploader/lv.js (1)
3-3: LGTM! Social source language now matches the locale.
The change from 'en' to 'lv' correctly aligns the social source language with the Latvian locale, improving the user experience for Latvian users.
locales/file-uploader/nl.js (1)
3-3: LGTM! Social source language now matches the locale.
The change from 'en' to 'nl' correctly aligns the social source language with the Dutch locale, maintaining consistency with other localization files.
locales/file-uploader/ca.js (2)
3-3: LGTM! Social source language now matches the locale.
The change from 'en' to 'ca' correctly aligns the social source language with the Catalan locale, consistent with other localization files.
1-3: Verify consistency across all locale files.
Let's ensure this change has been applied consistently across all localization files.
✅ Verification successful
All locale files have consistent language settings
The verification shows that all locale files in the locales/file-uploader/ directory have matching locale-id and social-source-lang values. Each file correctly uses its respective language code for both fields, including special cases like zh-TW.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if all locale files have their social-source-lang matching their locale-id
echo "Checking consistency of social-source-lang across all locale files..."
for file in $(fd -e js . locales/file-uploader/); do
echo "Checking $file:"
# Extract locale-id and social-source-lang values
locale_id=$(grep "'locale-id':" "$file" | awk -F"'" '{print $4}')
social_lang=$(grep "'social-source-lang':" "$file" | awk -F"'" '{print $4}')
if [ "$locale_id" = "$social_lang" ]; then
echo "✓ Matched: locale-id='$locale_id', social-source-lang='$social_lang'"
else
echo "✗ Mismatch: locale-id='$locale_id', social-source-lang='$social_lang'"
fi
echo "---"
done
Length of output: 17391
locales/file-uploader/pt.js (1)
3-3: LGTM! Language code matches locale.
The social source language is correctly set to 'pt' matching the Portuguese locale.
locales/file-uploader/ro.js (1)
3-3: LGTM! Language code matches locale.
The social source language is correctly set to 'ro' matching the Romanian locale.
locales/file-uploader/el.js (1)
3-3: LGTM! Language code matches locale.
The social source language is correctly set to 'el' matching the Greek locale.
Let's verify the consistency of this change across all locale files:
✅ Verification successful
All locale files have consistent language codes
After verifying all locale files in the locales/file-uploader/ directory, each file shows perfect consistency between its locale-id and social-source-lang values. The Greek locale file (el.js) and all other locale files maintain this consistency, confirming that the implementation is correct across the entire localization system.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that social-source-lang matches locale-id in all locale files
# Expected: Each file should have matching locale-id and social-source-lang values
echo "Checking consistency of locale settings..."
for file in $(fd . locales/file-uploader/ -e js); do
echo "Checking $file:"
# Extract and compare locale-id and social-source-lang
grep -A 1 "'locale-id':" "$file"
grep "'social-source-lang':" "$file"
echo "---"
done
Length of output: 12324
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (5)
locales/file-uploader/he.js (1)
121-122: LGTM with a minor suggestion for consistency.The Hebrew translations are accurate and properly follow RTL conventions. However, for better consistency with the existing translations in the file, consider using the feminine form "בחרי" instead of "בחר" to match the pattern used in other action verbs throughout the file.
Consider this alternative:
- 'select-all': 'בחר הכל', + 'select-all': 'בחרי הכל',locales/file-uploader/de.js (1)
30-31: Consider reordering the translation keys.While the German translations are accurate, these new keys are placed between 'selected-count' and 'upload-error', breaking the alphabetical ordering pattern seen in other locale files. Consider moving them to maintain consistent ordering across all locale files.
locales/file-uploader/ru.js (1)
123-124: Consider an alternative translation for 'deselect-all'.While both translations are grammatically correct, consider using "Снять выбор" or "Отменить выбор" for 'deselect-all' to maintain consistency with other UI elements and make it more concise. The current translation "Отменить выбор всех" is slightly verbose.
locales/file-uploader/uk.js (1)
123-124: LGTM! Consider reordering keys for better maintainability.The translations are grammatically correct and consistent with the Ukrainian language style. However, consider grouping these selection-related keys with other similar action keys for better maintainability.
Consider moving these keys near other selection-related translations (e.g., near the 'selected' key) to maintain a logical grouping:
selected: 'Вибрати всі', + 'select-all': 'Вибрати всі', + 'deselect-all': 'Скасувати вибір всіх', upload: 'Завантажити',locales/file-uploader/fr.js (1)
122-123: LGTM! Consider reordering keys for better maintainability.The French translations are accurate and grammatically correct. However, similar to the Ukrainian localization file, consider grouping these selection-related keys with other similar action keys.
Consider moving these keys near other selection-related translations for better maintainability:
selected: 'Sélectionné', + 'select-all': 'Tout sélectionner', + 'deselect-all': 'Tout désélectionner', upload: 'Télécharger',
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (33)
locales/file-uploader/ar.js(2 hunks)locales/file-uploader/az.js(2 hunks)locales/file-uploader/ca.js(2 hunks)locales/file-uploader/cs.js(2 hunks)locales/file-uploader/da.js(2 hunks)locales/file-uploader/de.js(1 hunks)locales/file-uploader/el.js(2 hunks)locales/file-uploader/es.js(1 hunks)locales/file-uploader/et.js(2 hunks)locales/file-uploader/fr.js(1 hunks)locales/file-uploader/he.js(1 hunks)locales/file-uploader/hy.js(2 hunks)locales/file-uploader/is.js(2 hunks)locales/file-uploader/it.js(1 hunks)locales/file-uploader/ja.js(2 hunks)locales/file-uploader/ka.js(2 hunks)locales/file-uploader/kk.js(2 hunks)locales/file-uploader/ko.js(2 hunks)locales/file-uploader/lv.js(2 hunks)locales/file-uploader/nb.js(2 hunks)locales/file-uploader/nl.js(2 hunks)locales/file-uploader/pl.js(2 hunks)locales/file-uploader/pt.js(2 hunks)locales/file-uploader/ro.js(2 hunks)locales/file-uploader/ru.js(1 hunks)locales/file-uploader/sk.js(2 hunks)locales/file-uploader/sr.js(2 hunks)locales/file-uploader/sv.js(2 hunks)locales/file-uploader/tr.js(1 hunks)locales/file-uploader/uk.js(1 hunks)locales/file-uploader/vi.js(2 hunks)locales/file-uploader/zh-TW.js(1 hunks)locales/file-uploader/zh.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (23)
- locales/file-uploader/az.js
- locales/file-uploader/vi.js
- locales/file-uploader/et.js
- locales/file-uploader/el.js
- locales/file-uploader/pt.js
- locales/file-uploader/pl.js
- locales/file-uploader/sk.js
- locales/file-uploader/ar.js
- locales/file-uploader/ko.js
- locales/file-uploader/ro.js
- locales/file-uploader/nb.js
- locales/file-uploader/ka.js
- locales/file-uploader/ca.js
- locales/file-uploader/ja.js
- locales/file-uploader/lv.js
- locales/file-uploader/cs.js
- locales/file-uploader/hy.js
- locales/file-uploader/nl.js
- locales/file-uploader/sv.js
- locales/file-uploader/da.js
- locales/file-uploader/is.js
- locales/file-uploader/kk.js
- locales/file-uploader/sr.js
🔇 Additional comments (5)
locales/file-uploader/it.js (1)
122-123: LGTM! The Italian translations are correctly implemented.
The new localization entries for select/deselect functionality follow the existing conventions and are properly translated to Italian.
Let's verify the consistency of these translations across other language files:
✅ Verification successful
✓ Translations are consistently implemented across all language files
The new select-all and deselect-all keys are properly implemented in all 34 locale files, including the Italian translations. Each language has appropriate translations that match their respective language conventions and semantics.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if all locale files have the new select-all and deselect-all keys
# and ensure consistent key naming across translations
echo "Checking translation consistency across locale files..."
for key in "select-all" "deselect-all"; do
echo "\nChecking presence of '${key}' in all locale files:"
rg "['\""]${key}['\"]\s*:" locales/file-uploader/
done
Length of output: 237
Script:
#!/bin/bash
# Let's try a simpler approach to check locale files
echo "Listing all locale files:"
fd . "locales/file-uploader"
echo -e "\nChecking for select-all in locale files:"
rg "'select-all':" locales/file-uploader/ || rg '"select-all":' locales/file-uploader/
echo -e "\nChecking for deselect-all in locale files:"
rg "'deselect-all':" locales/file-uploader/ || rg '"deselect-all":' locales/file-uploader/
Length of output: 5588
locales/file-uploader/zh.js (1)
119-120: LGTM! Translations are accurate and consistent.
The new translations for 'select-all' and 'deselect-all' are properly implemented in Simplified Chinese, following common UI patterns and maintaining consistency with existing translations.
locales/file-uploader/zh-TW.js (1)
119-120: LGTM! Traditional Chinese translations are accurate.
The new translations properly use Traditional Chinese characters and follow the established UI conventions for Traditional Chinese locales.
locales/file-uploader/tr.js (1)
119-120: LGTM! Turkish translations are accurate and consistent.
The translations for 'select-all' and 'deselect-all' are grammatically correct and maintain a consistent tone with the rest of the UI.
locales/file-uploader/es.js (1)
30-31: LGTM! Translations and placement are correct.
The Spanish translations are accurate and properly placed near other selection-related keys, maintaining a logical grouping in the file.
nd0ut
force-pushed
the
feat/new-social-sources
branch
from
f197394 to
66d273e
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (2)
blocks/Modal/modal.css (1)
Line range hint
6-7: Address the TODO comment about non-working backdropThe comment indicates that the backdrop background image isn't working but doesn't provide context about the issue. This could affect the visual appearance of themed modals.
Would you like me to:
- Create a GitHub issue to track this TODO?
- Help investigate why the backdrop background image isn't working?
Please provide more context about:
- Expected behavior
- Current behavior
- Any specific themes or conditions where this should work
blocks/ExternalSource/ExternalSource.js (1)
106-106: Remove unused parametervalThe parameter
valin the callback is defined but never used. Removing it can clean up the code and eliminate the warning.Apply this diff to remove the unused parameter:
-this.subConfigValue('localeName', (val) => { +this.subConfigValue('localeName', () => { this.setupL10n(); });🧰 Tools
🪛 GitHub Check: build
[warning] 106-106:
'val' is defined but never used
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (51)
blocks/CameraSource/CameraSource.js(0 hunks)blocks/CameraSource/camera-source.css(2 hunks)blocks/CloudImageEditorActivity/index.css(1 hunks)blocks/ExternalSource/ExternalSource.js(6 hunks)blocks/ExternalSource/MessageBridge.js(1 hunks)blocks/ExternalSource/buildStyles.js(0 hunks)blocks/ExternalSource/buildThemeDefinition.js(1 hunks)blocks/ExternalSource/external-source.css(3 hunks)blocks/ExternalSource/messages.js(0 hunks)blocks/ExternalSource/types.js(1 hunks)blocks/Modal/Modal.js(1 hunks)blocks/Modal/modal.css(1 hunks)blocks/StartFrom/start-from.css(1 hunks)blocks/UploadList/upload-list.css(1 hunks)blocks/UrlSource/url-source.css(0 hunks)blocks/themes/uc-basic/theme.css(2 hunks)demo/new-social-sources-test.html(1 hunks)locales/file-uploader/ar.js(1 hunks)locales/file-uploader/az.js(1 hunks)locales/file-uploader/ca.js(1 hunks)locales/file-uploader/cs.js(1 hunks)locales/file-uploader/da.js(1 hunks)locales/file-uploader/de.js(1 hunks)locales/file-uploader/el.js(1 hunks)locales/file-uploader/en.js(1 hunks)locales/file-uploader/es.js(1 hunks)locales/file-uploader/et.js(1 hunks)locales/file-uploader/fr.js(1 hunks)locales/file-uploader/he.js(1 hunks)locales/file-uploader/hy.js(1 hunks)locales/file-uploader/is.js(1 hunks)locales/file-uploader/it.js(1 hunks)locales/file-uploader/ja.js(1 hunks)locales/file-uploader/ka.js(1 hunks)locales/file-uploader/kk.js(1 hunks)locales/file-uploader/ko.js(1 hunks)locales/file-uploader/lv.js(1 hunks)locales/file-uploader/nb.js(1 hunks)locales/file-uploader/nl.js(1 hunks)locales/file-uploader/pl.js(1 hunks)locales/file-uploader/pt.js(1 hunks)locales/file-uploader/ro.js(1 hunks)locales/file-uploader/ru.js(1 hunks)locales/file-uploader/sk.js(1 hunks)locales/file-uploader/sr.js(1 hunks)locales/file-uploader/sv.js(1 hunks)locales/file-uploader/tr.js(1 hunks)locales/file-uploader/uk.js(1 hunks)locales/file-uploader/vi.js(1 hunks)locales/file-uploader/zh-TW.js(1 hunks)locales/file-uploader/zh.js(1 hunks)
💤 Files with no reviewable changes (4)
- blocks/UrlSource/url-source.css
- blocks/ExternalSource/buildStyles.js
- blocks/ExternalSource/messages.js
- blocks/CameraSource/CameraSource.js
🚧 Files skipped from review as they are similar to previous changes (41)
- locales/file-uploader/es.js
- locales/file-uploader/uk.js
- locales/file-uploader/zh-TW.js
- locales/file-uploader/fr.js
- locales/file-uploader/tr.js
- locales/file-uploader/it.js
- locales/file-uploader/ru.js
- locales/file-uploader/de.js
- locales/file-uploader/is.js
- locales/file-uploader/pl.js
- locales/file-uploader/pt.js
- locales/file-uploader/nb.js
- locales/file-uploader/az.js
- locales/file-uploader/da.js
- locales/file-uploader/ar.js
- locales/file-uploader/ja.js
- blocks/CameraSource/camera-source.css
- blocks/CloudImageEditorActivity/index.css
- locales/file-uploader/el.js
- locales/file-uploader/he.js
- locales/file-uploader/lv.js
- locales/file-uploader/ca.js
- locales/file-uploader/et.js
- locales/file-uploader/ro.js
- locales/file-uploader/sk.js
- locales/file-uploader/hy.js
- blocks/themes/uc-basic/theme.css
- blocks/StartFrom/start-from.css
- blocks/Modal/Modal.js
- locales/file-uploader/ko.js
- locales/file-uploader/nl.js
- locales/file-uploader/ka.js
- demo/new-social-sources-test.html
- locales/file-uploader/zh.js
- blocks/UploadList/upload-list.css
- locales/file-uploader/vi.js
- locales/file-uploader/sv.js
- locales/file-uploader/en.js
- locales/file-uploader/cs.js
- locales/file-uploader/kk.js
- locales/file-uploader/sr.js
🧰 Additional context used
🪛 GitHub Check: build
blocks/ExternalSource/ExternalSource.js
[warning] 106-106:
'val' is defined but never used
🔇 Additional comments (4)
blocks/Modal/modal.css (1)
27-28: LGTM! Improved responsive behavior with flexible sizing
The new width and height calculations using min() function and CSS variables provide better responsive behavior while preventing overflow. This is a good improvement over the previous fixed calculations.
Also applies to: 30-30
blocks/ExternalSource/MessageBridge.js (1)
12-14: Ensure message.type is a string before using includes()
To prevent potential runtime errors, verify that message.type is a string before calling includes(). This ensures the method functions correctly even if message.type is not a string.
blocks/ExternalSource/types.js (1)
1-142: Type definitions are comprehensive and well-structured
The type definitions enhance type safety and clarity across the application.
blocks/ExternalSource/buildThemeDefinition.js (1)
40-47: Consider adding validation for required theme properties
Currently, buildThemeDefinition silently skips undefined values. Validate essential theme properties to ensure a complete theme definition and prevent potential styling issues.
|
|
||
| /** @param {import('./types').OutputMessage} message */ | ||
| send(message) { | ||
| this._context.postMessage(message, '*'); |
There was a problem hiding this comment.
Avoid using wildcard '*' for targetOrigin in postMessage
Using '*' as the targetOrigin in postMessage can introduce security risks. Consider specifying the exact origin to ensure messages are only sent to trusted origins.
Apply this diff to specify the target origin:
send(message) {
- this._context.postMessage(message, '*');
+ const targetOrigin = '<trusted-origin>';
+ this._context.postMessage(message, targetOrigin);
}Replace '<trusted-origin>' with the actual origin string of the target window.
Committable suggestion skipped: line range outside the PR's diff.
| [uc-modal] > dialog:has(uc-external-source[active]) { | ||
| width: 100%; | ||
| height: 100%; |
There was a problem hiding this comment.
Check browser compatibility for :has() pseudo-class
The use of :has() in the selector dialog:has(uc-external-source[active]) may not be supported in all target browsers. Ensure that your supported browsers fully support this CSS feature or provide a fallback.
| this.bindL10n('counterText', () => | ||
| this.l10n('selected-count', { | ||
| count: message.selectedCount, | ||
| total: message.total, | ||
| }), | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid rebinding localization in handleSelectedFilesChange
Calling this.bindL10n inside handleSelectedFilesChange may cause multiple bindings with each call. Initialize the binding once in initCallback and update the values separately.
Move the binding to initCallback:
initCallback() {
super.initCallback();
+ this.bindL10n('counterText', () =>
+ this.l10n('selected-count', {
+ count: this.$.selectedCount,
+ total: this.$.total,
+ }),
+ );
// Existing code...
}Update handleSelectedFilesChange to set selectedCount and total:
this.set$({
isSelectionReady: message.isReady,
showSelectionStatus: message.isMultipleMode && message.total > 0,
couldSelectAll: message.selectedCount < message.total,
couldDeselectAll: message.selectedCount === message.total,
selectedList: message.selectedFiles,
+ selectedCount: message.selectedCount,
+ total: message.total,
});Committable suggestion skipped: line range outside the PR's diff.
nd0ut
force-pushed
the
feat/new-social-sources
branch
from
66d273e to
4d934da
Compare
nd0ut
force-pushed
the
feat/new-social-sources
branch
from
4d934da to
bd9b7cf
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
locales/file-uploader/fi.js (1)
95-103: Fix inconsistent indentationThe indentation in the validation messages section is inconsistent. Line 98 has extra spaces compared to line 95.
'files-count-limit-error-too-few': 'Olet valinnut {{total}} {{plural:file(total)}}. Vähintään {{min}} {{plural:file(min)}} vaaditaan.', 'files-count-limit-error-too-many': - 'Olet valinnut liian monta tiedostoa. {{max}} {{plural:file(max)}} on enimmäismäärä.', + 'Olet valinnut liian monta tiedostoa. {{max}} {{plural:file(max)}} on enimmäismäärä.',
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (34)
locales/file-uploader/ar.js(1 hunks)locales/file-uploader/az.js(1 hunks)locales/file-uploader/ca.js(1 hunks)locales/file-uploader/cs.js(1 hunks)locales/file-uploader/da.js(1 hunks)locales/file-uploader/de.js(1 hunks)locales/file-uploader/el.js(1 hunks)locales/file-uploader/es.js(1 hunks)locales/file-uploader/et.js(1 hunks)locales/file-uploader/fi.js(1 hunks)locales/file-uploader/fr.js(1 hunks)locales/file-uploader/he.js(1 hunks)locales/file-uploader/hy.js(1 hunks)locales/file-uploader/is.js(1 hunks)locales/file-uploader/it.js(1 hunks)locales/file-uploader/ja.js(1 hunks)locales/file-uploader/ka.js(1 hunks)locales/file-uploader/kk.js(1 hunks)locales/file-uploader/ko.js(1 hunks)locales/file-uploader/lv.js(1 hunks)locales/file-uploader/nb.js(1 hunks)locales/file-uploader/nl.js(1 hunks)locales/file-uploader/pl.js(1 hunks)locales/file-uploader/pt.js(1 hunks)locales/file-uploader/ro.js(1 hunks)locales/file-uploader/ru.js(1 hunks)locales/file-uploader/sk.js(1 hunks)locales/file-uploader/sr.js(1 hunks)locales/file-uploader/sv.js(1 hunks)locales/file-uploader/tr.js(1 hunks)locales/file-uploader/uk.js(1 hunks)locales/file-uploader/vi.js(1 hunks)locales/file-uploader/zh-TW.js(1 hunks)locales/file-uploader/zh.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (33)
- locales/file-uploader/tr.js
- locales/file-uploader/uk.js
- locales/file-uploader/it.js
- locales/file-uploader/ru.js
- locales/file-uploader/pt.js
- locales/file-uploader/he.js
- locales/file-uploader/fr.js
- locales/file-uploader/is.js
- locales/file-uploader/da.js
- locales/file-uploader/pl.js
- locales/file-uploader/nl.js
- locales/file-uploader/el.js
- locales/file-uploader/cs.js
- locales/file-uploader/zh-TW.js
- locales/file-uploader/nb.js
- locales/file-uploader/sk.js
- locales/file-uploader/ro.js
- locales/file-uploader/hy.js
- locales/file-uploader/sv.js
- locales/file-uploader/ca.js
- locales/file-uploader/ka.js
- locales/file-uploader/az.js
- locales/file-uploader/de.js
- locales/file-uploader/es.js
- locales/file-uploader/zh.js
- locales/file-uploader/vi.js
- locales/file-uploader/ja.js
- locales/file-uploader/ar.js
- locales/file-uploader/lv.js
- locales/file-uploader/ko.js
- locales/file-uploader/et.js
- locales/file-uploader/sr.js
- locales/file-uploader/kk.js
🔇 Additional comments (4)
locales/file-uploader/fi.js (4)
104-119: Great addition of accessibility strings!
The new accessibility strings are well-structured and comprehensive, covering:
- ARIA labels for editor tabs and controls
- Clear status messages for file items
- Proper use of interpolation for dynamic content
120-121: LGTM: Selection control strings added
The new selection control strings are properly translated and align with the PR objectives.
2-3: Verify the impact of social source language change
The change of 'social-source-lang' from 'fi' to 'en' might affect social media integration functionality. This change appears to be intentional based on the AI summary, but we should verify its impact.
✅ Verification successful
Setting social-source-lang to 'en' is consistent with the majority of locale files
The change from 'fi' to 'en' for the Finnish locale aligns with the common pattern across the codebase. Most locales use 'en' as their social source language, with only a few exceptions for major languages (zh, uk, ru, tr, it, he, fr, es, de) that use their native language codes. This change appears to be a deliberate standardization.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if this change is consistent across other locale files
echo "Checking social-source-lang values across locale files:"
rg "'social-source-lang':" locales/file-uploader/
Length of output: 2214
2-121: Verify completeness against other language files
While the translations look good, we should ensure all keys are consistent across language files to avoid missing translations.
✅ Verification successful
Based on the script output, no missing keys were reported for any of the locale files when compared against the Finnish (fi.js) locale file. The script successfully checked all translation files in the locales/file-uploader directory and found that all keys present in fi.js exist in other locale files.
All translation keys are consistent across locale files
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Extract all keys from the Finnish locale
echo "Extracting keys from Finnish locale..."
keys_fi=$(rg "^ '[^']+'" locales/file-uploader/fi.js | sed "s/ '//" | sed "s/'.*//")
# Check for missing keys in other locale files
echo "Checking for missing keys in other locale files..."
for locale_file in locales/file-uploader/*.js; do
if [ "$locale_file" != "locales/file-uploader/fi.js" ]; then
echo "Checking $locale_file..."
while IFS= read -r key; do
if ! rg -q " '${key}'" "$locale_file"; then
echo "Missing key in $locale_file: $key"
fi
done <<< "$keys_fi"
fi
done
Length of output: 68542
Summary by CodeRabbit
Release Notes
New Features
MessageBridgeclass for better communication between web contexts.Improvements
uc-camera-source,uc-cloud-image-editor-activity, anduc-external-sourcecomponents.Bug Fixes