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

Update next branch with changes from main #10572

Merged
merged 6 commits into from
Sep 20, 2023
Merged

Conversation

sophschneider
Copy link
Contributor

@sophschneider sophschneider commented Sep 19, 2023

Updates next branch with latest changes from main.

  1. git merge main into a next branch
  2. Resolve conflicts by manually going through main commits and next commits to gain context and take what was needed
  3. Solved any typescript errors including net new migrated components (e.g. I renamed net new HorizontalStacks to InlineStack, any net new button props I manually migrated)
  4. Consolidated net new se23 styles
  5. Made sure storybook ran as expected
  6. Made sure doc website ran as expected

🚨 Please verify that changes in the last linked commit above look as expected in storybook (see CI for link)

@github-actions github-actions bot added the cla-needed Added by a bot. Contributor needs to sign the CLA Agreement. label Sep 19, 2023
@github-actions github-actions bot removed the cla-needed Added by a bot. Contributor needs to sign the CLA Agreement. label Sep 19, 2023
@sophschneider sophschneider added the 🤖Skip Comment Check Skip the migrator comment CI check label Sep 19, 2023
@@ -35,25 +35,13 @@

.Item {
position: relative;
// stylelint-disable-next-line polaris/z-index/declaration-property-value-allowed-list -- override z-index
z-index: unset;
margin-top: 0;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changes from this commit into main

cc: @mattkubej would you be able to tophat and make sure these changes still work on our feature branch? I think it's just the focus ring on separated buttons right?

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 noticed in the storybook regression tests that pressed segmented buttons no longer have a dark right side border from this change. Is that intentional?

Before After
Screenshot 2023-09-19 at 3 44 45 PM Screenshot 2023-09-19 at 3 44 58 PM

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 tophatted pagination and segmented buttons and they match the main storybook 👍 we may have to fix the right shadow/border in a follow up PR to main

@@ -22,4 +22,41 @@
background-color: var(--p-color-bg-strong-active);
}
}

&.table {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mattkubej I removed the se23 when merging this into our feature branch, are the styles still working as expected?

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 tophatted and it seems to match main 👍

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, just seeing this note. Removing should be good. Thanks!

@@ -9,18 +9,4 @@
display: flex;
justify-content: center;
align-items: center;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@gwyneplaine @jesstelford I think these styles were missed in the IndexTable cleanup, are they good to remove? The se23 styles seem to just be reseting the styles I deleted above but I haven't tophatted

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 tophatted against main, it all looks good (the same, no regressions :) )

polaris-tokens/src/themes/base/color.ts Show resolved Hide resolved
Copy link
Member

@lgriffee lgriffee left a comment

Choose a reason for hiding this comment

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

Token changes look good! 🪙✨🎉

Copy link
Contributor

@kyledurand kyledurand 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 have context on all these changes but the ones I do have look good 👍

Copy link
Contributor

@mrcthms mrcthms left a comment

Choose a reason for hiding this comment

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

Changes pertaining to Filters look good to me!

Copy link
Contributor

@laurkim laurkim left a comment

Choose a reason for hiding this comment

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

💯 🙌

@sophschneider
Copy link
Contributor Author

I have a few questions about the UI test changes but I've confirmed they match main so I've accepted them

@sophschneider
Copy link
Contributor Author

sophschneider commented Sep 20, 2023

@aaronccasanova @lgriffee I've merged in your latest token changes (since you last reviewed Laura), lmk if they look ok!

@sophschneider
Copy link
Contributor Author

I'm going to merge this PR to unblock some folks, if there are any more concerns we can fix with a follow up PR!

@sophschneider
Copy link
Contributor Author

sophschneider commented Sep 20, 2023

FYI @yurm04 @jesstelford the modal website examples are looking a bit off when closing the modal. Not sure if this a regression bc the example isn't working on the base branch rn but maybe we should fix with a follow up PR. Might have to do with wrapping the examples in Frame

edit: made an issue here #10586

Screenshot 2023-09-20 at 10 05 58 AM

@sophschneider sophschneider merged commit d78a7e2 into next Sep 20, 2023
12 checks passed
@sophschneider sophschneider deleted the sophie/merge-main branch September 20, 2023 14:13
@lgriffee
Copy link
Member

@aaronccasanova @lgriffee I've merged in your latest token changes (since you last reviewed Laura), lmk if they look ok!

Looked at the latest token changes added and they look good!

AnnaCheba pushed a commit to AnnaCheba/polaris that referenced this pull request Apr 22, 2024
Updates `next` branch with latest changes from `main`.
1. `git merge main` into a `next` branch
2. Resolve conflicts by manually going through `main` commits and `next`
commits to gain context and take what was needed
3. Solved any typescript errors including net new migrated components
(e.g. I renamed net new `HorizontalStack`s to `InlineStack`, any net new
button props I manually migrated)
4. Consolidated net new se23 styles
5. Made sure storybook ran as expected
6. Made sure doc website ran as expected


> 🚨 Please verify that changes in the last linked commit above look as
expected in storybook (see CI for link)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🤖Skip Comment Check Skip the migrator comment CI check
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants