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

reset all:unset issues #296

Closed
kevinmitch14 opened this issue Feb 13, 2024 · 18 comments
Closed

reset all:unset issues #296

kevinmitch14 opened this issue Feb 13, 2024 · 18 comments
Labels
ecosystem Incompatibility with something else

Comments

@kevinmitch14
Copy link

Feel free to change Issue name. It looks like the changes made in reset.css via #276 has caused some unexpected changes. I believe this is closer to native HTML behaviour anyway, but maybe should be flagged in changelog?

Before:
Screenshot 2024-02-13 at 09 03 33

After:
Screenshot 2024-02-13 at 09 04 32

Code Snippet: FormLabel is using Radix Label under the hood.

<FormItem className="space-y-2">
  <FormLabel>Industry</FormLabel>
  <FormControl>
    <SelectRoot
      onValueChange={field.onChange}
      defaultValue={field.value}
      name={field.name}
    >
      <SelectTrigger
        placeholder="Select an industry"
        className="w-full"
      />
      <SelectContent
        position="popper"
        onPointerDownOutside={field.onBlur}
      >
        {INDUSTRY_OPTIONS.map((item) => (
          <SelectItem value={item} key={item}>
            {upperFirst(item)}
          </SelectItem>
        ))}
      </SelectContent>
    </SelectRoot>
  </FormControl>
  <FormMessage />
</FormItem>
@kevinmitch14
Copy link
Author

I've also noticed changes in DropdownMenuItem, specifically here

This was changed from justify-content: space-between;, resulting in changes like below.

Before:
Screenshot 2024-02-13 at 09 20 30

After:
Screenshot 2024-02-13 at 09 21 26

No issues on my end, I just added space-between back again but just raising to your attention!

@kevinmitch14
Copy link
Author

I've found that the following, fixes my initial issue...

<SelectTrigger
  placeholder="Select an industry"
- className="w-full"
+ style={{ width: "100%" }}
/>

@kevinmitch14
Copy link
Author

Also:

<Card className="min-h-[200px]" asChild>
  <Link href={"/"}>
    <Flex
      justify={"center"}
      direction={"column"}
      align={"center"}
      height={"100%"}
    >
      <ArchiveIcon className="h-5 w-5" />
      <Text weight={"medium"}>One</Text>
    </Flex>
  </Link>
</Card>

Again, using style={{}} instead of className works, but I am using TailwindCSS.

Before:
Screenshot 2024-02-13 at 11 06 13

After:
Screenshot 2024-02-13 at 11 05 34

@kevinmitch14 kevinmitch14 changed the title <Select /> reset 'regression' reset all:unset issues Feb 13, 2024
@vladmoroz
Copy link
Contributor

vladmoroz commented Feb 13, 2024

Thanks, this is helpful—we'll double check these, all of that is going to be under a major 3.0.0 release

What does your FormItem style look like?

The dropdown menu change is the only one that's intended:
#296 (comment)

In the updated version it's optimised for icons next to the text (e.g. icon first, then text with a small gap between them), as the previous one just didn't consider nesting your own icons. You can still do that space-between manually if you need the previous behaviour. You could probably use the CheckboxItem part there though which already handles the checkmark and the layout?

@kevinmitch14
Copy link
Author

FormItem is basically just a div with className="space-y-2".

Good idea on the CheckboxItem, never thought of that so will try this out. I've currently reverted back to v2.0.3 but will play around with it v3-rc.

Happy to keep this issue open and document any other unexpected changes I notice throughout my app.

@vladmoroz
Copy link
Contributor

Looking into the Select Trigger and Card issues you mentioned – the CSS classes are still forwarded to the right elements, which makes me think that in your case the Tailwind utilities just don’t overwrite the styles for whatever reasons. All the related styles are of 0,1,0 specificity, just like Tailwind.

Can you check whether your Tailwind utilities are applied after Radix Themes CSS? e.g. via #109 (comment).

Happy to keep this issue open and document any other unexpected changes I notice throughout my app.

Sure, feel free to add more here.

@kevinmitch14
Copy link
Author

kevinmitch14 commented Feb 14, 2024

CSS Setup 1:

@tailwind base;
@tailwind components;
@tailwind utilities;

@import "@radix-ui/themes/styles.css";

Radix 2.0.3 + tailwindcss@insiders (MY CURRENT SETUP)
There is no unset: all, everything works as expected. No specificity issues

Radix 3.0.0-rc + tailwindcss@insiders
min-height: line 631 (not applied)
radix reset: line 7k-ish

Radix 3.0.0-rc + [email protected]
min-height: line 27k (applied)
radix reset: line 4.5k


CSS Setup 2:

@tailwind base;

@import "@radix-ui/themes/styles.css";

@tailwind components;
@tailwind utilities;

Radix 3.0.0-rc + tailwindcss@insiders
min-height: line 33k(applied)
radix reset: line 6k


Changing CSS import order seems to have done the trick, I was not having specificity issues such as when using <Button type="submit>...</Button> but I'm using the insiders version of tailwind so that's probably why. 🤔

Will keep an eye on the new setup and report any findings

@vladmoroz
Copy link
Contributor

From when I tested it, Setup 1 and Setup 2 were equivalent in the latest stable TW version, but if it works, it works. Not sure why you haven't seen other issues—at the very least, @tailwind utilities should have been unable to overwrite any existing Radix Themes styles.

Changing CSS import order seems to have done the trick, I was not having specificity issues such as when using <Button type="submit>...</Button> but I'm using the insiders version of tailwind so that's probably why. 🤔

Could be because of this change tailwindlabs/tailwindcss#12735 being already in the insiders version.

I'll close this for now but feel free to report any other issues here or in separate reports.

@vladmoroz vladmoroz added the ecosystem Incompatibility with something else label Feb 14, 2024
@kevinmitch14
Copy link
Author

@vladmoroz I don't think RadioCardGroupItem respects Inset. Is this expected?

<div className="w-fit">
  <Card>
    <Inset side={"all"} clip={"border-box"}>
      <Image
        alt={`Photo`}
        width={300}
        height={200}
        key={"media.id"}
        // className="aspect-video h-auto max-w-[100%] rounded-2 bg-cover bg-no-repeat align-middle"
        src={
          "https://images.unsplash.com/photo-1682686581221-c126206d12f0?q=80&w=1470&auto=format&fit=crop&ixlib=rb-4.0.3&ixid=M3wxMjA3fDF8MHxwaG90by1wYWdlfHx8fGVufDB8fHx8fA%3D%3D"
        }
      />
    </Inset>
  </Card>
</div>
<div className="w-fit">
  <RadioCardGroupRoot>
    <RadioCardGroupItem key={"1"} value={"1"}>
      <Inset side={"all"} clip={"border-box"}>
        <Image
          alt={`Photo`}
          width={300}
          height={200}
          key={"media.id"}
          // className="aspect-video h-auto max-w-[100%] rounded-2 bg-cover bg-no-repeat align-middle"
          src={
            "https://images.unsplash.com/photo-1682686581221-c126206d12f0?q=80&w=1470&auto=format&fit=crop&ixlib=rb-4.0.3&ixid=M3wxMjA3fDF8MHxwaG90by1wYWdlfHx8fGVufDB8fHx8fA%3D%3D"
          }
        />
      </Inset>
    </RadioCardGroupItem>
  </RadioCardGroupRoot>
</div>

Result: Comparing with Card
Screenshot 2024-02-15 at 15 25 50

@vladmoroz
Copy link
Contributor

Good catch, will take a note to make Inset work there

@kevinmitch14
Copy link
Author

Also, does it make sense to use display: flex on RadioCardGroupItem?

Context: I am adding image name under the image. I see Card uses display: block which to me seems a little more intuitive for adding more than one element to RadioCardGroupItem.

@kevinmitch14
Copy link
Author

kevinmitch14 commented Feb 22, 2024

@vladmoroz

Something else I came across. It looks the new dialog scroll overlay setup is causing this? I can remove these styles but then margin: auto in .rt-BaseDialogScrollPadding cannot be overridden from the Dialog itself afaik

Context: I am trying to use a dialog positioned at the top of the screen.

<DialogRoot>
  <DialogTrigger>
    <IconButton variant="soft">
      <TextAlignJustifyIcon />
    </IconButton>
  </DialogTrigger>
  <DialogContent className="absolute top-4 m-4" style={{ maxWidth: "95%" }}>
    {...}
  </DialogContent>
</DialogRoot>

Before:
Screenshot 2024-02-22 at 09 20 29

After:
Screenshot 2024-02-22 at 09 22 40

@vladmoroz
Copy link
Contributor

@kevinmitch14 what are you trying to achieve with these style overrides?

className="absolute top-4 m-4" style={{ maxWidth: "95%" }}

@kevinmitch14
Copy link
Author

@kevinmitch14 what are you trying to achieve with these style overrides?

className="absolute top-4 m-4" style={{ maxWidth: "95%" }}

Context: I am trying to use a dialog positioned at the top of the screen.

@vladmoroz
Copy link
Contributor

vladmoroz commented Feb 22, 2024

You probably want something like this:

className="absolute left-1/2 -translate-x-1/2" style={{ maxWidth: "95%" }}

That m-4 you are setting is not factored into the maxWidth: "95%" and causes the layout to overflow.

We'll take a look into providing better options for positioning the dialog though so that you don't have to do a custom style that is potentially fragile with updates

@kevinmitch14
Copy link
Author

In the old version there was no overflow with m-4 added. But since using rc version, I've found that as soon as position: absolute; is added, there is an overflow. (Even when margin and max-width is removed. I should have mentioned that initially)

Your solution works well though

@kevinmitch14
Copy link
Author

@vladmoroz, unrelated to v3 but should <TextField color="gray" /> have a gray active outline? I've noticed that it will inherit the theme accent color, not the color prop that is passed to it.

Is this by design or an oversight? --color-focus-root/--focus-8 (rc version) is the CSS var being used to style the outline. On soft variants, --accent-8 is being used.


Do the classic / surface variants have any differences when using color="gray" vs. inheriting accent?

Screenshot 2024-02-28 at 14 45 42

@benoitgrelard
Copy link
Contributor

No this is intentional to maintain a consistent focus outline.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ecosystem Incompatibility with something else
Projects
None yet
Development

No branches or pull requests

3 participants