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.

Sign up for GitHub

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

chore(weave): add width to autocomplete #2500

Merged
merged 4 commits into from
Sep 27, 2024
Merged

chore(weave): add width to autocomplete #2500

Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
30a60ab
add width to autocomplete
mbarrramsey Sep 26, 2024
aae2788
.
mbarrramsey Sep 26, 2024
bf86bc7
.
mbarrramsey Sep 26, 2024
109d047
rename to autocompleteRef
mbarrramsey Sep 26, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 5 additions & 3 deletions weave-js/src/components/Form/AutoComplete.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,9 +40,9 @@ const getStyles = (props: AdditionalProps) => {
paddingBottom: '0px !important',
fontSize: FONT_SIZES[size],
fontFamily: 'Source Sans Pro',
minWidth: '100px',
color: MOON_800,
maxWidth: props.maxWidth ? `${props.maxWidth}px` : '100%',
minWidth: props.minWidth ? `${props.minWidth}px` : '100px',
'& fieldset': {
borderColor: MOON_250,
},
Expand All @@ -59,7 +59,7 @@ const getStyles = (props: AdditionalProps) => {
opacity: 1,
},
'& .MuiInputBase-input': {
padding: '0px', // Adjust padding as needed
padding: '0px',
minHeight: `${HEIGHTS[size]} !important`,
},
},
Expand Down Expand Up @@ -145,15 +145,17 @@ type AdditionalProps = {
size?: SelectSize;
isDarkMode?: boolean;
maxWidth?: number;
minWidth?: number;
hasInputValue?: boolean;
inputRef?: React.MutableRefObject<HTMLDivElement | null>;
Copy link
Contributor

@connieelee connieelee Sep 26, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

suggestion: inputRef naming seems misleading if it's actually a div element under there? maybe should be named something else

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe inputDivRef? or autocompleteDivRef?

Copy link
Contributor

Choose a reason for hiding this comment

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

we should probably leave out "input" entirely. if you inspect the elements, there's an actual input element nested inside, but this ref is definitely not being attached to that.

have you tried React.forwardRef()?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did try the forwardRef, but I don't think it works well with generics 🤔 see https://github.com/wandb/core/pull/24247/files#r1777844929. We could try using the wrapper approach but it seems like the custom name is similar?

Copy link
Contributor

Choose a reason for hiding this comment

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

ahh i see. maybe something like containerRef or autocompleteRef seems fine then!

};

export const AutoComplete = <Option,>(
props: AutocompleteProps<Option, boolean, boolean, boolean> & AdditionalProps
) => {
return (
<ThemeProvider theme={getStyles(props)}>
<Autocomplete {...props} />
<Autocomplete ref={props.inputRef} {...props} />
</ThemeProvider>
);
};
Loading