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

Autofill popover offset in reshaped 2.4.4 #183

Closed
rauloaida opened this issue Oct 13, 2023 · 16 comments
Closed

Autofill popover offset in reshaped 2.4.4 #183

rauloaida opened this issue Oct 13, 2023 · 16 comments

Comments

@rauloaida
Copy link

rauloaida commented Oct 13, 2023

Describe the bug
The position of the autofill popover is offset when compared with input field. Horizontal position of autofill depends on window width. Bug not present in 2.4.2, introduced in 2.4.4 to the best of my digging.

Expected behavior
Autofill popover is centered on the input field being clicked.

Screenshots
2 4 4 - min reproducible error

safari

To Reproduce
Open your IDE of choice.
Clone my example repo OR go to the vercel app I deployed.
Go to branch autofill-popover-off-center-bug
Run npm install && npm run dev.
In your browser, open the localhost url & port.
Click on TextField component to focus and select an autofill value. If no autofill value is present, I found Safari allows you to generate a random autofill email address to use.
Observe error. See attached screenshots/recording for reference.
Versions:

Reshaped 2.4.4
Chrome v117.0.5938.92
Safari 16.5.1

@blvdmitry
Copy link
Contributor

Thanks, I can reproduce this and know why it happens now:
We're adding negative margins for the input to keep it clickable when slots are used, which then affects the position of the native autocomplete

Going to find a way around it and ship a patch

@blvdmitry
Copy link
Contributor

I've published 2.4.5 with the fix in case this is blocking, but it also includes minor updates for the default theme values related to the next minor release. You can either just update to 2.4.5 and try if the fix is working or if keeping current theme values is important - you can keep them as a custom theme. They will be updated for the Figma library in 2.5.0 as well.

@rauloaida
Copy link
Author

rauloaida commented Oct 15, 2023

Thanks for the update, just switched for 2.4.4 to 2.4.5 in my code example. Still looks a little wonky, see attached screenshots. My guess it that the autofill popover is justify: center instead of justify: start. This is only noticeable on a smaller width input field.

expected actual

@blvdmitry
Copy link
Contributor

And this is a native autocomplete, right? Does it behave correctly in the case of replacing TextField with a native html input?

@rauloaida
Copy link
Author

You are correct! I guess this is the default behavior, so should be good to go. See attached screenshot of native html only.
htmlonly

Is there any indication when 2.5.0 will be coming out? checked the roadmap page but I don't see any date pinned to it. Thanks!

@blvdmitry
Copy link
Contributor

One task left for the release there, so planning to be done with it around this Wed/Thu.
Thanks for checking the native implementation out!

@rauloaida
Copy link
Author

@blvdmitry getting this in my implementation. Was reshaped/cli/theming/definitions/base removed in 2.4.4 > 2.4.5 ?

Screenshot 2023-10-18 at 10 46 34 AM

@blvdmitry
Copy link
Contributor

blvdmitry commented Oct 19, 2023

Hey, this was a file private to the package (no real way to mark that way unfortunately). But you can also import this definition from reshaped/themes now. It will be added to the docs in 2.5.0

image

@rauloaida
Copy link
Author

Upgraded to Reshaped 2.5.0 today and changed the import from

import baseDefinitions from 'reshaped/cli/theming/definitions/base'
to:
import baseDefinitions from 'reshaped/themes'

Now getting the error below, I've rebuilt the theme just in case.
Screenshot 2023-10-30 at 6 20 20 PM

The code snippet where this is used:

'@media': {
			[`screen and (min-width: ${baseDefinitions.viewport.l.minPx}px)`]: {
				position: 'static',
				transition: 'unset',
				width: 'unset',
				translate: 'unset',
				zIndex: 'unset'
			}
		}

Not sure if the docs have updated or if you have any suggestions, currently not building on 2.4.4 and above ;(

@blvdmitry blvdmitry reopened this Oct 30, 2023
@blvdmitry
Copy link
Contributor

Let me check this, I think viewports might be stored in a separate object that was not a part of the js theme definition, so I'll make sure to include it and release a patch with it

@rauloaida
Copy link
Author

Let me know when you have an update! or if there is anything else that I can provide from my end

@blvdmitry
Copy link
Contributor

Just checking it again and it actually seems to exist there: https://codesandbox.io/s/objective-colden-p894rr?file=/src/App.tsx

The types are not great though so I'm going to make sure they work as expected in a patch. Does output 900 in your case as well?

@blvdmitry
Copy link
Contributor

Updated the theme definition types in 2.5.5

@rauloaida
Copy link
Author

Unless I'm missing something it looks like 'base' is not being exported correctly, it errors in your codesandbox if you give it a few seconds to run the typescript checks. I get the same error when trying to use 2.5.5 in my own repo. See attached.

Screenshot 2023-11-01 at 5 16 14 AM

@blvdmitry
Copy link
Contributor

blvdmitry commented Nov 1, 2023

It's baseThemeDefinition, seems like my link ended up in a weird state

@rauloaida
Copy link
Author

Works, thank you!

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

No branches or pull requests

2 participants