Skip to content

Conversation

@AlexAndBear
Copy link
Contributor

@AlexAndBear AlexAndBear commented Dec 4, 2025

Description

image

Related Issue

How Has This Been Tested?

  • test environment:
  • test case 1:
  • test case 2:
  • ...

Types of changes

  • Bugfix
  • Enhancement (a change that doesn't break existing code or deployments)
  • Breaking change (a modification that affects current functionality)
  • Technical debt (addressing code that needs refactoring or improvements)
  • Tests (adding or improving tests)
  • Documentation (updates or additions to documentation)
  • Maintenance (like dependency updates or tooling adjustments)

@AlexAndBear AlexAndBear changed the title feat: add inlineLabel and hasBorder props to OcTextInput feat: add inlineLabel and hasBorder props to OcTextInput and OCSelect Dec 4, 2025
AlexAndBear and others added 2 commits December 4, 2025 12:10
Co-authored-by: Jannik Stehle <50302941+JammingBen@users.noreply.github.com>
Co-authored-by: Jannik Stehle <50302941+JammingBen@users.noreply.github.com>
Copy link
Contributor

@JammingBen JammingBen left a comment

Choose a reason for hiding this comment

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

👍

@AlexAndBear AlexAndBear marked this pull request as ready for review December 4, 2025 13:02
Copilot AI review requested due to automatic review settings December 4, 2025 13:02
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR adds two new props (inlineLabel and hasBorder) to OcTextInput and OcSelect components to provide more flexible styling options. The inlineLabel prop displays labels next to input fields instead of above them, while hasBorder controls whether the input has a visible border.

Key Changes:

  • Added inlineLabel and hasBorder props to both OcTextInput and OcSelect components with appropriate default values
  • Applied these new props in the mail compose view to create a cleaner, more compact form layout
  • Updated component documentation with examples demonstrating the new customization options

Reviewed changes

Copilot reviewed 11 out of 11 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
packages/design-system/src/components/OcTextInput/OcTextInput.vue Added inlineLabel and hasBorder props with styling logic
packages/design-system/src/components/OcSelect/OcSelect.vue Added inlineLabel and hasBorder props with styling logic
packages/web-app-mail/src/views/MailCompose.vue Applied new props to form fields with custom border styling
packages/design-system/docs/components/OcTextInput/OcTextInput.md Added documentation for inline label feature
packages/design-system/docs/components/OcTextInput/customization.vue Created example demonstrating customization options
Multiple snapshot files Updated test snapshots to reflect new default prop values

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

</div>
<div class="px-4">
<div class="py-3">
<div class="py-2 mb-2 border-b border-role-outline-variant">
Copy link

Copilot AI Dec 4, 2025

Choose a reason for hiding this comment

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

The border styling classes 'py-2 mb-2 border-b border-role-outline-variant' are duplicated across multiple form fields (lines 20, 36, 44, 52, 59). Consider extracting this repeated pattern into a CSS class or applying the border through the parent container to reduce duplication.

Copilot uses AI. Check for mistakes.
'pl-6': !!readOnly,
'pr-6': showClearButton
'pr-6': showClearButton,
'border-none outline-none bg-transparent': !hasBorder
Copy link

Copilot AI Dec 4, 2025

Choose a reason for hiding this comment

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

Using '!important' implicitly through 'border-none' and 'outline-none' may override necessary focus states. Consider using a more specific selector or ensure focus styles are still accessible when hasBorder is false.

Suggested change
'border-none outline-none bg-transparent': !hasBorder
'border-none bg-transparent focus:ring-2 focus:ring-primary': !hasBorder

Copilot uses AI. Check for mistakes.
.vs__dropdown-toggle {
border: none !important;
outline: none !important;
background-color: transparent !important;
Copy link

Copilot AI Dec 4, 2025

Choose a reason for hiding this comment

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

Removing the outline with '!important' will eliminate focus indicators, which are critical for keyboard navigation and accessibility. Ensure alternative focus styling is provided when hasBorder is false.

Suggested change
background-color: transparent !important;
background-color: transparent !important;
// Provide alternative focus indicator for accessibility
&:focus {
box-shadow: 0 0 0 2px var(--oc-role-outline);
}

Copilot uses AI. Check for mistakes.
:::

### Customization
`OcTextInput` is highly customizable
Copy link

Copilot AI Dec 4, 2025

Choose a reason for hiding this comment

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

The customization section description is incomplete. Add a period at the end of the sentence and expand the description to explain what customization options are demonstrated in the example.

Suggested change
`OcTextInput` is highly customizable
`OcTextInput` is highly customizable. The example below demonstrates how you can customize the component by adding custom icons, adjusting input styles, and modifying placeholder text to better fit your application's needs.

Copilot uses AI. Check for mistakes.
@AlexAndBear AlexAndBear merged commit 199c060 into main Dec 4, 2025
28 checks passed
@AlexAndBear AlexAndBear deleted the issues/1679 branch December 4, 2025 14:54
@openclouders openclouders mentioned this pull request Dec 4, 2025
1 task
@openclouders openclouders mentioned this pull request Dec 15, 2025
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants