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

Dropdown Expandable - QA review tweaks from Laura and Orion #1145

Merged
merged 10 commits into from
Oct 19, 2023

Conversation

chris-cedrone-cengage
Copy link
Collaborator

Issue: # 1014

What I did

  • CSS tweaks to component including outline and padding adjustments.
  • Added doc example for 'defaultIndex', fixed missing references.
  • Removed unnecessary prop inclusions.

Checklist

  • changeset has been added
  • Pull request description is descriptive
  • I have made corresponding changes to the documentation
  • New and existing unit tests pass locally with my changes
  • [N/A] I have added tests that prove my fix is effective or that my feature works

How to test

  • Make sure all Codesandbox examples load properly.
  • Verify padding and outline fixes are appropriately tuned.
  • Confirm the 'iconPosition' doesn't change layout when set to 'left' or 'none' on 'DropdownExpandableMenuGroup'.
  • Confirm the 'disabled' prop only works for 'DropdownExpandableMenuItem'.
  • Rejoice

@changeset-bot
Copy link

changeset-bot bot commented Oct 18, 2023

🦋 Changeset detected

Latest commit: 7790ab5

The changes in this PR will be included in the next version bump.

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@github-actions
Copy link
Contributor

@github-actions
Copy link
Contributor

@github-actions
Copy link
Contributor

@github-actions
Copy link
Contributor

@github-actions
Copy link
Contributor

@github-actions
Copy link
Contributor

@github-actions
Copy link
Contributor

@github-actions
Copy link
Contributor

Copy link
Contributor

@orion-cengage orion-cengage left a comment

Choose a reason for hiding this comment

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

I don't think the following feedback was addressed:

  • There should only be 8px of padding at the top and bottom of the dropdown container. It looks like there is some extra padding being applied on a div inside that container, making the padding actually look more like 12px or 16px.

Image

@github-actions
Copy link
Contributor

@github-actions
Copy link
Contributor

const StyledDiv = styled.div`
padding: ${props => props.theme.spaceScale.spacing02} 0;
`;
const StyledDiv = styled.div``;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Since it's not styled... can we remove this and just use <div> in line 154?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

🤘

@github-actions
Copy link
Contributor

@github-actions
Copy link
Contributor

@github-actions
Copy link
Contributor

@github-actions
Copy link
Contributor

@chris-cedrone-cengage chris-cedrone-cengage merged commit fa88fa7 into dev Oct 19, 2023
1 check passed
@silvalaura silvalaura deleted the fix/expandableDropdown branch July 25, 2024 17:44
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.

3 participants