Skip to content

Conversation

@Chang-Chen
Copy link

@Chang-Chen Chang-Chen commented Sep 16, 2025

closed #226

Summary by CodeRabbit

  • New Features

    • Added a maxWidth prop to BubbleList to limit the component’s maximum width (default: 'none'), enabling better horizontal sizing control.
  • Documentation

    • Updated BubbleList docs (ZH) to include the new maxWidth option.
    • Enhanced Storybook with a maxWidth control and default example.
  • Refactor

    • Improved style handling to use reactive sizing, providing more predictable and consistent rendering without manual DOM updates.

@coderabbitai
Copy link

coderabbitai bot commented Sep 16, 2025

Walkthrough

Adds a new optional maxWidth prop to BubbleList, wires it via reactive CSS variables instead of DOM-side updates, updates styles to respect max-width, adjusts docs and Storybook args, and extends TypeScript types accordingly. Removes onMounted/watch style initialization in favor of a computed style object bound to the root element.

Changes

Cohort / File(s) Summary
Component logic: BubbleList
packages/core/src/components/BubbleList/index.vue
Adds public prop maxWidth (default 'none'). Replaces initStyle/onMounted/watch with computed elBubbleListStyleVars for CSS vars: --el-bubble-list-max-width, --el-bubble-list-max-height, --el-bubble-list-btn-size. Binds :style to root. Minor guard/indent tweaks.
Styles: BubbleList
packages/core/src/components/BubbleList/style.scss
Applies max-width: var(--el-bubble-list-max-width) to .el-bubble-list.
Types: BubbleList
packages/core/src/components/BubbleList/types.d.ts
Adds maxWidth?: string to BubbleListProps.
Docs
apps/docs/zh/components/bubbleList/index.md
Documents new public prop maxWidth (String, optional, default 'none').
Storybook
packages/core/src/stories/BubbleList/BubbleList.stories.ts
Adds argType control for maxWidth (text) and default arg maxWidth: 'none'.

Sequence Diagram(s)

sequenceDiagram
  autonumber
  actor User
  participant Storybook as Storybook Args
  participant BubbleList as BubbleList Component
  participant Vue as Computed Style
  participant DOM as Root Element
  participant CSS as SCSS Vars

  User->>Storybook: Configure maxWidth (e.g., "640px" or "none")
  Storybook->>BubbleList: Pass prop maxWidth
  BubbleList->>Vue: Compute elBubbleListStyleVars<br/>(--el-bubble-list-max-width = maxWidth)
  Vue->>DOM: Bind :style with CSS variables
  DOM->>CSS: Apply max-width via var(--el-bubble-list-max-width)
  note over BubbleList,Vue: onMounted/watch removed<br/>reactive binding handles updates
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

Suggested reviewers

  • HeJiaYue520
  • WengJianFei

Poem

I nudge a bubble, sleek and bright,
Now width behaves, just set it right.
No watchers hop—just vars that flow,
A tidy warren, css in tow.
Thump-thump! I cheer with ears upright—
MaxWidth arrives; the UI’s tight. 🐇✨

Pre-merge checks and finishing touches

❌ Failed checks (1 inconclusive)
Check name Status Explanation Resolution
Out of Scope Changes Check ❓ Inconclusive While the maxWidth addition is implemented, the PR also removes the previous initStyle/onMounted/watch logic that set document-root CSS variables and replaces it with a component-scoped computed style, which is a broader scoping/implementation change beyond the issue request and could affect other components relying on global CSS variables, so it is unclear from the current diff whether that refactor is safe. Request the author to confirm the intended change in CSS variable scoping, run visual regression or integration tests for components that might rely on document-root variables, and either document this behavioral change in the PR or separate the refactor into its own commit/PR to make the scope explicit.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check ✅ Passed The title clearly states the main change (adding a maxWidth property to BubbleList) and references issue #226, so it accurately reflects the PR's purpose; however it contains a duplicated word "添加添加" and a noisy trailing issue reference which make it less concise.
Linked Issues Check ✅ Passed The PR implements the linked issue #226: it adds an optional maxWidth prop (types and component), updates styles to apply max-width via a CSS variable, and updates docs and Storybook args with a default of 'none', which satisfies the coding objective to provide a configurable max width for BubbleList.
Docstring Coverage ✅ Passed No functions found in the changes. Docstring coverage check skipped.
✨ Finishing touches
  • 📝 Generate Docstrings
🧪 Generate unit tests
  • Create PR with unit tests
  • Post copyable unit tests in a comment

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

🧹 Nitpick comments (8)
packages/core/src/components/BubbleList/style.scss (1)

11-12: Add CSS variable fallbacks to avoid invalid values when props are empty.

Using var(--x, fallback) makes styles resilient to SSR/auto-import timing and empty-string props.

-  max-width: var(--el-bubble-list-max-width);
-  max-height: var(--el-bubble-list-max-height);
+  max-width: var(--el-bubble-list-max-width, none);
+  max-height: var(--el-bubble-list-max-height, 100%);
packages/core/src/components/BubbleList/types.d.ts (1)

17-19: Type width/height as CSSProperties to improve DX and accept numbers.

This keeps ‘none’ working while enabling values like 680 (px) if you later coerce numbers.

+ import type { CSSProperties } from 'vue';
   export interface BubbleListProps<
     T extends BubbleListItemProps = BubbleListItemProps
   > {
     list: T[];
     autoScroll?: boolean;
-    maxWidth?: string;
-    maxHeight?: string;
+    maxWidth?: CSSProperties['maxWidth'];
+    maxHeight?: CSSProperties['maxHeight'];
apps/docs/zh/components/bubbleList/index.md (1)

54-54: Clarify accepted formats with examples.

Explicitly listing valid CSS lengths avoids confusion.

-| `maxWidth`            | String                                         | 否                                      | 'none'                                         | 氣泡列表容器的最大宽度,默认值为 `none`。                                                                                                                                       |
+| `maxWidth`            | String                                         | 否                                      | 'none'                                         | 氣泡列表容器的最大宽度,支持任意合法 CSS 长度(如 `680px`、`42rem`、`min(90vw, 720px)`),默认 `none`。                                                                         |
packages/core/src/stories/BubbleList/BubbleList.stories.ts (1)

13-13: Stories LGTM; consider adding a fixed-width variant to visually validate the prop.

Add another story to showcase a constrained width:

export const FixedWidthDemo: Story = {
  args: {
    ...BubbleListDemo.args,
    maxWidth: '680px',
  },
};

Also applies to: 27-27

packages/core/src/components/BubbleList/index.vue (4)

34-38: Normalize/fallback style variables to avoid empty CSS values.

Prevents invalid var() resolutions if props are empty and aligns with docs’ default behavior.

-const elBubbleListStyleVars = computed(() => ({
-  '--el-bubble-list-max-width': props.maxWidth,
-  '--el-bubble-list-max-height': props.maxHeight,
-  '--el-bubble-list-btn-size': `${props.btnIconSize}px`
-}));
+const elBubbleListStyleVars = computed(() => ({
+  '--el-bubble-list-max-width': props.maxWidth || 'none',
+  '--el-bubble-list-max-height': props.maxHeight || '100%',
+  '--el-bubble-list-btn-size': `${Number(props.btnIconSize) || 24}px`,
+}));

91-92: Fix lint: newline after if (antfu/if-newline).

-  if (!container) return;
+  if (!container)
+    return;

94-95: Fix lint: newline after if (antfu/if-newline).

-  if (index >= bubbles.length) return;
+  if (index >= bubbles.length)
+    return;

168-170: Address indent rule by avoiding short‑circuit and using if with newline.

-    default:
-      props.triggerIndices.includes(index) &&
-        emits('complete', instance, index);
+    default:
+      if (props.triggerIndices.includes(index))
+        emits('complete', instance, index);
       break;
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between e3ce64d and c60879f.

📒 Files selected for processing (5)
  • apps/docs/zh/components/bubbleList/index.md (1 hunks)
  • packages/core/src/components/BubbleList/index.vue (6 hunks)
  • packages/core/src/components/BubbleList/style.scss (1 hunks)
  • packages/core/src/components/BubbleList/types.d.ts (1 hunks)
  • packages/core/src/stories/BubbleList/BubbleList.stories.ts (2 hunks)
🧰 Additional context used
🪛 ESLint
packages/core/src/components/BubbleList/index.vue

[error] 91-91: Expect newline after if

(antfu/if-newline)


[error] 94-94: Expect newline after if

(antfu/if-newline)


[error] 169-169: Expected indentation of 6 spaces

(style/indent-binary-ops)

🔇 Additional comments (2)
packages/core/src/components/BubbleList/index.vue (2)

13-13: Default ‘none’ is sensible for maxWidth.


202-202: Binding CSS vars via :style is the right move—prop changes reactively update layout.

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.

BubbleList 建议添加一个maxWidth属性

1 participant