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

Avatar in AppBar rendering incorrectly in Safari #2055

Closed
trentpeterson opened this issue Sep 18, 2023 · 10 comments · Fixed by #2333
Closed

Avatar in AppBar rendering incorrectly in Safari #2055

trentpeterson opened this issue Sep 18, 2023 · 10 comments · Fixed by #2333
Assignees
Labels
bug Something isn't working
Milestone

Comments

@trentpeterson
Copy link

trentpeterson commented Sep 18, 2023

Current Behavior

In Safari on MacOS (Version 16.6 (18615.3.12.11.2), Ventura 13.5.2) with Skeleton 2.1.0, Skeleton Tailwind 0.2.0, and Tailwind 3.3.3, I'm observing that the Avatar component causes the entire contents of the AppBar to render in the middle of the viewport when placed in the trail slot.

This issue sounds very similar to the issue described in #1699.

Expected Behavior

The contents of the AppBar should be properly vertically centered within it as it is in Chrome and in previous versions of Skeleton.

Steps To Reproduce

Locally, this reproduces with the following snippet:

<AppBar>
  <svelte:fragment slot="lead">
    <strong class="text-xl uppercase">test</strong>
  </svelte:fragment>
  <svelte:fragment slot="trail">
    <Avatar initials="sk" />
  </svelte:fragment>
</AppBar>

I was also able to reproduce in a hokey way by manually inserting the rendered HTML from an Avatar component into an AppBar's rendered HTML within the Skeleton docs.

Link to Reproduction / Stackblitz

Is there a StackBlitz with newer 2.x Skeleton and 4.x Svelte (One on the homepage appears to be 1.x/3.x)? Thought I'd ask before I put one together.

More Information

I managed to workaround the issue by overriding the aspect-square class applied to the div rendered by an Avatar with .avatar.aspect-square { aspect-ratio: auto }.

@trentpeterson trentpeterson added the bug Something isn't working label Sep 18, 2023
@endigo9740
Copy link
Contributor

endigo9740 commented Sep 18, 2023

Since it looks like you're testing with a modern version of Tailwind and Safari, can you also share the contents of your tailwind.config.[ts|js] please.

Is there a StackBlitz with newer 2.x Skeleton

Not yet, but soon per: #1995

@trentpeterson
Copy link
Author

Thanks for the quick reply.

tailwind.config.ts:

import { join } from 'path'
import type { Config } from 'tailwindcss'
import { skeleton } from '@skeletonlabs/tw-plugin'
import { planshipTheme } from './src/lib/styles/theme.ts'

const config = {
  darkMode: 'class',
  content: [
    './src/**/*.{html,js,svelte,ts}',
    join(require.resolve(
      '@skeletonlabs/skeleton'),
      '../**/*.{html,js,svelte,ts}'
    )
  ],
  theme: {
    extend: {},
  },
  plugins: [
    require('@tailwindcss/forms'),
    skeleton({
      base: true,
      themes: {
        custom: [ planshipTheme ]
      }
    })
  ]
} satisfies Config

export default config

Not yet, but soon per:
#1995

👍

@endigo9740
Copy link
Contributor

endigo9740 commented Sep 18, 2023

Any reason you have base: true explicitly set in the Skeleton plugin settings? Those styles should be on by default, so it's really only needed if which to specify false. Any change of behavior when removing that? (you may need to restart your dev server)

I'm going to ping @AdrianGonz97 (the author of the new plugin) to to confirm this will work as expected - that base styles will remain enabled when set true.

@endigo9740
Copy link
Contributor

endigo9740 commented Sep 18, 2023

@trentpeterson it just occurred to me we have some default styles that affect the Trail slot. The expectation is most folks will want a series of horizontal items in that slot. For example:

button button button

This COULD be the reason the avatar is affected in this manner. Can you try wrapping the avatar in a simple div without any other classes applied. Like so:

<AppBar>
  <svelte:fragment slot="lead">
    <strong class="text-xl uppercase">test</strong>
  </svelte:fragment>
  <svelte:fragment slot="trail">
    <div>
        <Avatar initials="sk" />
    </div>
  </svelte:fragment>
</AppBar>

If this resolves your issue I may look to adjust the behavior of this component and the slot styles. What we have now may be too opinionated.

@trentpeterson
Copy link
Author

Any reason you have base: true explicitly set in the Skeleton plugin settings? Those styles should be on by default, so it's really only needed if which to specify false. Any change of behavior when removing that? (you may need to restart your dev server)

I had base set to false in order to remove the custom scrollbar behavior in core, but I set it to true while debugging this issue. Removing the explicit assignment and restarting the dev server had no effect on the AppBar+Avatar behavior.

This COULD be the reason the avatar is affected in this manner. Can you try wrapping the avatar in a simple div without any other classes applied. Like so:

Just tried this verbatim and the AppBar content still displays in the middle of the page. Removing the aspect-square class from the figure corrects the issue as before.

I also tried moving the Avatar to the lead slot and had the same result. Also also, I commented out all of my custom CSS in app.postcss just to make sure I didn't forget about some custom styling, but the issue persists.

@trentpeterson
Copy link
Author

Interestingly, if I remove h-full from the svg without removing aspect-square from the figure, the issue also goes away.

@endigo9740
Copy link
Contributor

endigo9740 commented Sep 19, 2023

Interestingly, if I remove h-full from the svg without removing aspect-square from the figure, the issue also goes away.

Not sure I follow - this is the first mention about an SVG. Not sure how that comes into play here.

Just FYI to anyone reviewing this, here's what the issue looks like with the snippet provided in the original post:

Screenshot 2023-09-19 at 11 20 53 AM

Compared to the avatar implemented in an App Bar that lives within an App Shell:

Screenshot 2023-09-19 at 11 21 54 AM

While Trent's aspect ratio class change may work, I don't think that's an ideal solution here. Users shouldn't have to modify a default Tailwind class for these components to work as expected.

I've tested a few things with a brand new CLI-generated project without the PurgeCSS plugin. Unfortunately none of these changes have helped:

  • Removing all traces of the App Shell - including the app.html styles and app.postcss styles.
  • I've tested both with and without the root layout <slot />
  • I've tested a wrapping div for the avatar in the trail slot
  • I've testing specifying a set width for the avatar (though it should have a default width)

The issue is very specifically the combination of Avatar + App Bar slots. Adding the avatar to either the Lead or Trail slots causes the issue to occur. I'm leaning towards it being an issue with the App Bar styles, not the avatar. Though I have nothing to sustain that theory yet.

@trentpeterson I'm limited on time today so I'll have to circle back on this. But it's definitely an odd issue, and definitely one we'll try to resolve if we can.

@endigo9740
Copy link
Contributor

endigo9740 commented Sep 19, 2023

@trentpeterson as a temporary work around here's a solution that does appear to work if you're adverse to the App Shell:

<!-- /src/routes/+layout.svelte -->

<script lang="ts">
  import "../app.postcss";
  import { AppBar, Avatar } from "@skeletonlabs/skeleton";
</script>

<main class="grid grid-rows-[auto_1fr]">
  <AppBar>
    <svelte:fragment slot="lead">
      <Avatar initials="sk" />
    </svelte:fragment>
    <svelte:fragment slot="trail">
      <strong class="text-xl uppercase">test</strong>
    </svelte:fragment>
  </AppBar>
  <div>
    <slot />
  </div>
</main>

So far best I can tell the App Bar needs some kind of defined wrapping parent element that squishes it into size vertically, otherwise it (visually) tries to fill the height of the container. Using grid styling accomplishes this a lot like the App Shell.

Note that this is using the same prerequisite styles that App Shell uses, which makes the default height of the body fill the viewport:
https://www.skeleton.dev/components/app-shell#prerequisites

@trentpeterson
Copy link
Author

trentpeterson commented Sep 19, 2023

Interestingly, if I remove h-full from the svg without removing aspect-square from the figure, the issue also goes away.

Not sure I follow - this is the first mention about an SVG. Not sure how that comes into play here.

I'm manipulating the resulting HTML in the inspector, which contains an svg when initials are used. To be fair, it also happens if I remove the h-full class from the img when src is provided. Basically, there appears to be some conflict within Safari between aspect-ratio set to 1/1 on the parent while the child has a height specified as anything besides auto. No idea if this is helpful...

<figure class="avatar flex aspect-square text-surface-50 font-semibold justify-center items-center overflow-hidden isolate bg-surface-400-500-token w-16  rounded-full   " data-testid="avatar">
  <svg class="avatar-initials w-full h-full" viewBox="0 0 512 512">
    <text x="50%" y="50%" dominant-baseline="central" text-anchor="middle" font-weight="bold" font-size="150" class="avatar-text fill-token">
      SK
    </text>
  </svg>
</figure>

While Trent's aspect ratio class change may work, I don't think that's an ideal solution here. Users shouldn't have to modify a default Tailwind class for these components to work as expected.

Absolutely agree. It's working for me, specifically right at this moment, but it's definitely a hasty hack to move forward. Granted, users wouldn't have to modify/override Tailwind if Skeleton didn't specify the aspect-square class on the figure, but I imagine there's a good reason for having it there in certain circumstances.

@trentpeterson I'm limited on time today so I'll have to circle back on this. But it's definitely an odd issue, and definitely one we'll try to resolve if we can.

I appreciate the attention you've already provided. I have a workaround for now so I'm good.

@trentpeterson as a temporary work around here's a solution that does appear to work if you're adverse to the AppShell

Thanks for this! I'll give it a shot. I do already use the prerequisites from AppShell on body etc, but this parent grid container is new.

I'll also follow up on Discord as I have some general questions about App Shell usage, etc.

@szethh
Copy link

szethh commented Sep 25, 2023

I have the same issue. The Avatar causes all elements in the AppBar to appear in the middle of the page, in macOS and iOS (safari).

I am using an AppShell:

<AppShell slotPageContent="m-8">
	<svelte:fragment slot="header"
		><AppBar>
			<svelte:fragment slot="lead">
				<a href="/" class="flex items-center gap-4">
					<img src="/favicon.png" height="1rem" alt="logo" class="w-16" />

					<span class="text-xl font-extralight font-heading-token">App title</span>
				</a>
			</svelte:fragment>

			<svelte:fragment slot="trail">
				<LightSwitch />


				<div> // tried with & without a parent div - no change
					<Avatar
						border="border-4 border-surface-300-600-token hover:!border-primary-500"
						cursor="cursor-pointer"
						{initials}
					/>
				</div>

			</svelte:fragment>
		</AppBar>
	</svelte:fragment>

	<slot />
</AppShell>

The issue is fixed by removing h-full from the svg:

<figure class="avatar flex aspect-square text-surface-50 font-semibold justify-center items-center overflow-hidden isolate bg-surface-400-500-token w-16 border-4 border-surface-300-600-token hover:!border-primary-500 rounded-full  cursor-pointer " data-testid="avatar">
-     <svg class="avatar-initials w-full h-full" viewBox="0 0 512 512">
+     <svg class="avatar-initials w-full" viewBox="0 0 512 512">
        <text x="50%" y="50%" dominant-baseline="central" text-anchor="middle" font-weight="bold" font-size="150" class="avatar-text fill-token">CP</text>
    </svg>
</figure>

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants