formaat-design / reshaped

Community repository for storing examples, reporting issues and tracking roadmap
https://reshaped.so
97 stars 3 forks source link

Autofill popover offset in reshaped 2.4.4 #183

Closed rauloaida closed 8 months ago

rauloaida commented 8 months ago

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 commented 8 months ago

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 commented 8 months ago

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 commented 8 months ago

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 commented 8 months ago

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

rauloaida commented 8 months ago

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 commented 8 months ago

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 commented 8 months ago

@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 commented 8 months ago

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 commented 8 months ago

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 commented 8 months ago

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 commented 8 months ago

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

blvdmitry commented 8 months ago

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 commented 8 months ago

Updated the theme definition types in 2.5.5

rauloaida commented 8 months ago

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 commented 8 months ago

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

rauloaida commented 8 months ago

Works, thank you!