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

[dds-tabs-extended-media] Add a story for mixed tabs content to highlight grid issues #10880

Open
wants to merge 13 commits into
base: v1
Choose a base branch
from

Conversation

marcelojcs
Copy link
Contributor

@marcelojcs marcelojcs commented Aug 25, 2023

Related Ticket(s)

Jira ADCMS-3155
#10298

Description

The linked issue here reflects a broader issue wherein inline padding for tab / accordion contents varies in such a way that it can hang in the grid gutter, or even indent too far.

This PR adds a story under "Tabs extended - with media" with a range of mixed contents, reflecting those which are commonly used in AEM specfically, but also serve as a good general sample. This was based off a Figma mockup of the various allowed components within "Tabs extended - with media" on AEM.

Changelog

New

  • added story "Tabs extended - with media / With Mixed Contents" to demonstrate variety of tab contents

Changed

  • removed styles that no longer apply given a recent change that shuffled accordian markup out of the <dds-tabs-extended-media> shadow DOM.

@ibmdotcom-bot
Copy link
Contributor

ibmdotcom-bot commented Aug 25, 2023

@ibmdotcom-bot
Copy link
Contributor

ibmdotcom-bot commented Aug 25, 2023

@emyarod emyarod changed the title fix(tabs-extented) ajust padding for nested componnents fix(tabs-extended): adjust padding for nested components Aug 25, 2023
Copy link
Member

@emyarod emyarod left a comment

Choose a reason for hiding this comment

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

it looks like there is a syntax error? missing a closing brace

@ibmdotcom-bot
Copy link
Contributor

ibmdotcom-bot commented Sep 28, 2023

@ibmdotcom-bot
Copy link
Contributor

ibmdotcom-bot commented Sep 28, 2023

@m4olivei
Copy link
Contributor

m4olivei commented Sep 29, 2023

I've brought over the "Tabs extended - with media / With Mixed Content" story from @jkaeser 's PR (#10353) here. There are a few things that we can observe now:

1. Issue reported in #10298 has been resolved by recent changes.

The original issue, is no longer present, none of the contents are hanging in the gutter. All of the tab contents are getting the default padding that comes from Carbon web component styles. These styles used to be overidden for the <dds-tabs-extended-media> component here. Those styles no longer apply since this change. That change pushed a lot of the accordion markup from <dds-tabs-extended-media> to <dds-tab>, such that the styles no longer apply. I've removed these styles.

Prior to change (v1.49.0):
image

After change (Canary)
image

  1. Some tab contents have magin / padding that interfere with us acheiving a clean inline grid layout

The change effectively acheived what @oliviaflory was calling for in this comment on the original issue. Although the original specific issue is resolved, we still have the larger issue @oliviaflory pointed out where some tab contents have magin / padding that interfere with us acheiving a clean inline grid layout.

  1. Content item horizontal padding too much within tabs extended

In the original issue, @oliviaflory noted that Content item horizontal had padding to fit well within tab contents. Now that the tab contents container carries the right padding, tabs with content item horizontal have too much overall padding and are now mis-aligned. See screenshot taken from Canary:

image

  1. Mixed content and variable witdth tab contents.

When you have mixed content in the tabs, the tab contents have variable width and switching between the tabs, expands and contracts the overall component. You don't notice this with the existing story examples b/c the tab contents are homogenous (see below from deploy preview). This is b/c of some display: flex declarations to accomodate a section heading. AEM gets around this by setting a custom style like this:

.tabs dds-tabs-extended-media::part(tabs-extended) {
    width: 100%;
}

I think we can mitigate this issue by reserving some of the display: flex style rules to only when there is a section heading in play.

Here is the bug from deploy preview:

Monosnap.screencast.2023-09-29.12-48-13.mp4

@m4olivei m4olivei changed the title fix(tabs-extended): adjust padding for nested components [dds-tabs-extended-media] Add a story for mixed tabs content to highlight grid issues Sep 29, 2023
@m4olivei
Copy link
Contributor

m4olivei commented Sep 29, 2023

Discussed with @jkaeser and @marcelojcs. Here's a proposal for how to move this forward:

  • The inconsistencies in how components do / don't define inline padding probably deserves a discussion, breaking down how we handle fixing on a component by component basis. @oliviaflory had mentioned this was going to be addressed in v2 way back in April. Was that done? If so, maybe we consider this lost for v1?
  • There is value in the story added here, to at least highlight the various issues and set the goal posts for getting to an eventual fix around various issues noted in my previous comment.
  • I've removed styles that no longer apply. That's worthwhile cleanup.
  • The linked issues should be closed and various issues with narrower scopes should be opened if there is continued interest in fixing this stuff. Note that from an AEM perspective, there are various (non-ideal) work arounds in place, so it's not an immediate issue as far as we're aware.

cc @andy-blum @kennylam

@andy-blum andy-blum changed the base branch from main to v1 November 30, 2023 18:45
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

Successfully merging this pull request may close these issues.

None yet

6 participants