Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixed dropdown blur and unified components #9062

Merged
merged 16 commits into from
Dec 17, 2024
Merged

Conversation

lucasbordeau
Copy link
Contributor

@lucasbordeau lucasbordeau commented Dec 13, 2024

  • Removed disableBlur property from dropdown because it is no longer needed since there's only one OverlayContainer component so there can be only one blur at a time.
  • Removed blur CSS properties from every component that used it because one standalone OverlayContainer is able to handle all cases if placed properly.
  • Also removed disableBackgroundBlur property from SingleRecordSelect
  • Removed FieldInputOverlay and FieldTextAreaOverlay components that were a first attempt to create something like an OverlayContainer
  • Used new unified OverlayContainer in RecordInlineCell and RecordTableCell
  • Fixed ScrollWrapper so that it works well both for dropdown with non overflowing content and dropdown with overflowing content.
  • Removed export default value on SearchVariablesDropdown as it is not used in this codebase
  • Refactored SearchVariablesDropdown function as component anti-pattern
  • Refactored SearchVariablesDropdownFieldItems UI problems with separator and missing ScrollWrapper behavior
  • Refactored SearchVariablesDropdownObjectItems with UI problems with separator and missing ScrollWrapper behavior
  • Fixed blur bug on Firefox due to wrong placement of the element that had the CSS property. Blur works on Firefox it it's on the container that has the highest level in the tree.
  • Fixed bug in ActivityTargetInlineCell by removing an unnecessary container component StyledSelectContainer
  • Unified problems of field height with a new common component FieldInputContainer, instead of putting width and height at the wrong abstraction level, width and height are a field's concern not a dropdown, overlay or low-level input concern.
  • Fixed block editor dropdown with new OverlayContainer
  • Aligning field dropdown with their anchor on inline and table cells, there are still many small pixel misalignments that give a low quality impression.
  • Fixed FormDateFieldInput that was missing OverlayContainer

Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

PR Summary

This PR unifies backdrop filter handling across dropdowns and overlay components by introducing a new StyledOverlayContainer component, particularly addressing Firefox-specific issues with stacked backdrop filters.

  • Added new StyledOverlayContainer in /ui/layout/overlay/components/StyledOverlayContainer.tsx as the single source for backdrop filter CSS
  • Removed disableBlur prop from all dropdown-related components to centralize blur control
  • Removed FieldInputOverlay.tsx and OVERLAY_BACKGROUND constant in favor of the new unified approach
  • Adjusted positioning offsets in RecordInlineCellEditMode and RecordTableCellEditMode for better alignment
  • Consolidated dropdown menu styling by removing redundant backdrop filters and border styles from individual components

55 file(s) reviewed, 12 comment(s)
Edit PR Review Bot Settings | Greptile

<TextInput
placeholder={fieldDefinition.metadata.placeHolder}
autoFocus
value={draftValue?.toString() ?? ''}
Copy link
Contributor

Choose a reason for hiding this comment

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

style: Consider validating the input string before converting to ensure it's a valid number

@@ -9,6 +9,7 @@ const StyledInlineCellButtonContainer = styled.div`
align-items: center;
display: flex;
`;

export const RecordInlineCellButton = ({ Icon }: { Icon: IconComponent }) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

logic: component name in export doesn't match filename (RecordInlineCellEditButton.tsx vs RecordInlineCellButton)

date={internalValue ?? new Date()}
onChange={handleChange}
onMouseSelect={handleMouseSelect}
clearable={clearable ? clearable : false}
Copy link
Contributor

Choose a reason for hiding this comment

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

style: redundant ternary - clearable ? clearable : false can be simplified to !!clearable

dropdownHeightComponentStateV2.atomFamily({
instanceId: dropdownId,
}),
)
Copy link
Contributor

Choose a reason for hiding this comment

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

logic: onClickOutside is called inside handleClickableComponentClick which seems incorrect - this callback should typically be for clicks outside the dropdown, not for clicks on the clickable component itself

({ snapshot, set }) =>
(availableHeight: any, elements: any) => {
flushSync(() => {
const elementRect = elements.floating.getBoundingClientRect();
Copy link
Contributor

Choose a reason for hiding this comment

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

style: using querySelector('#root') is fragile - consider passing boundary as a prop or finding a more robust way to determine the boundary

Comment on lines 3 to 6
const StyledDropdownMenu = styled.div<{
disableBlur?: boolean;
disableBorder?: boolean;
width?: `${string}px` | `${number}%` | 'auto' | number;
}>`
Copy link
Contributor

Choose a reason for hiding this comment

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

logic: disableBlur prop was removed but still accepting disableBorder - should probably remove disableBorder too since border styling was moved

Comment on lines 46 to 50
<SearchVariablesDropdown
inputId={inputId}
onVariableSelect={onVariableSelect}
disabled={disabled}
/>
Copy link
Contributor

Choose a reason for hiding this comment

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

logic: multiline prop is passed to component but not used after removing StyledSearchVariablesDropdownContainer wrapper. This may break multiline functionality.

Comment on lines 73 to 74
if (disabled === true) {
return (
Copy link
Contributor

Choose a reason for hiding this comment

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

style: strict equality comparison with boolean is unnecessary, can be simplified to 'if (disabled)'

<StyledDropdownComponentsContainer>
{renderSearchVariablesDropdownComponents()}
</StyledDropdownComponentsContainer>
!isDefined(selectedStep) ? (
Copy link
Contributor

Choose a reason for hiding this comment

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

style: this conditional rendering could be simplified using a switch statement or early returns for better readability

) : (
<MenuItem
key="no-steps"
onClick={() => {}}
Copy link
Contributor

Choose a reason for hiding this comment

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

style: empty onClick handler could be omitted since it's not doing anything

@charlesBochet
Copy link
Member

Found 3 bugs while doing the functional review

image

@lucasbordeau lucasbordeau changed the title Fix dopwon blur Fix dropdown blur and unified components Dec 13, 2024
@lucasbordeau lucasbordeau changed the title Fix dropdown blur and unified components Fixed dropdown blur and unified components Dec 13, 2024
@lucasbordeau lucasbordeau enabled auto-merge (squash) December 17, 2024 11:00
@lucasbordeau lucasbordeau enabled auto-merge (squash) December 17, 2024 14:26
@lucasbordeau lucasbordeau merged commit 860dec3 into main Dec 17, 2024
22 checks passed
@lucasbordeau lucasbordeau deleted the fix/dropdown-blur branch December 17, 2024 14:28
@lucasbordeau
Copy link
Contributor Author

Fixes #8840

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: ✅ Done
Development

Successfully merging this pull request may close these issues.

2 participants