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

chore: remove deprecated code for next major #2536

Open
wants to merge 1 commit into
base: 19.0.0
Choose a base branch
from

Conversation

agliga
Copy link
Contributor

@agliga agliga commented Jan 24, 2025

Went through the code and removed all deprecated code.
Please verify these are okay to remove. If there are additional ones (like fullscreen or drawer dialog), will add them too!

Things I did not change because I was not sure if its okay to remove, but happy to remove:
https://github.com/eBay/skin/blob/master/src/sass/page-grid/page-grid.scss#L57-L63 subgrid
https://github.com/eBay/skin/blob/master/src/sass/variables/_variables.scss#L30 variables (both because it might add too many breaking changes if we add dialogs on top of it, but also which ones need to be removed?)

@agliga agliga self-assigned this Jan 24, 2025
Copy link

changeset-bot bot commented Jan 24, 2025

🦋 Changeset detected

Latest commit: fc79149

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

This PR includes changesets to release 1 package
Name Type
@ebay/skin Major

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

@agliga agliga requested a review from HenriqueLimas January 24, 2025 16:31
// Deprecated, remove next major
// This is tied with handle, once we remove handle this needs to be removed
.lightbox-dialog__window--expanded {
height: 95%;
Copy link
Member

Choose a reason for hiding this comment

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

Is this accompanied by a change so that the dialog is essentially always in this expanded state? Even if we do that, I'm not convinced that every dialog should be almost fullscreen

Copy link
Contributor

Choose a reason for hiding this comment

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

Agree. We need sensible defaults for our major breakpoints. Almost fullscreen would not be a sensible default for large screens. Design system should have defaults those specified. But also we probably need the capacity for apps to fine tune those when needed (using component level CSS variables?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I removed the modifier which expanded the dialog to be expanded to almost full screen when the handle is clicked. Not sure what this comment is trying to say. The default is normal size

Copy link
Contributor

@ArtBlue ArtBlue left a comment

Choose a reason for hiding this comment

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

Let's remove them from storybook, too.

image

Otherwise, looks great!

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.

4 participants