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

2929 upgrade carbon #2932

Merged
merged 47 commits into from
Nov 11, 2021
Merged

2929 upgrade carbon #2932

merged 47 commits into from
Nov 11, 2021

Conversation

kevinsperrine
Copy link
Collaborator

@kevinsperrine kevinsperrine commented Oct 6, 2021

Closes #2929
Closes #2437

Summary

10.40

  • MenuButtons are now opened in a portal, so tests using container to query for classes needed to be switched to document.body

10.41

  • Pagination buttons were reduced in size causing style breaks
    • .bx--pagination__button was reduced from 3rem to 2.5rem
    • added size parameter to SimplePagination and defaulted it to lg

10.42

  • No new test failures

10.43

  • title attributes were added to the selected item in Dropdowns, so tests using screen.getByRole('button', { name: 'Size Small (4x1)' }) have now changed, b/c the title attribute doesn't match the text content that it used to screen.getByRole('button', { name: 'Size Small (4x1) Open menu' }) in one case
  • Custom dropdown-based components (SimpleIconDropdown, ColorDropdown, etc) will now have a title attribute that is [Object object], because carbon is now using itemToString to add a title to the dropdown. This doesn't prevent the dropdowns from working, but they become less accessible. An issue has been opened requesting the ability to render the selected item with itemToElement instead of always using itemToString

10.44

  • The prop wrapperClassName for Checkbox will be deprecated in V11 in favor of className. className will then be placed on the outer wrapper.
  • The handleClick prop for ClickableTile has been deprecated in favor of onClick. It will be removed in the next major release.

10.45

  • no new test failures

10.46

  • no new test failures

10.47

Acceptance Test (how to verify the PR)

Regression Test (how to make sure this PR doesn't break old functionality)

  • as with all carbon upgrades we check all stories
  • with specific attention paid to:
    • any items using pagination to make sure the buttons are the correct size
    • SimpleIconDropdown, IconDropdown, ColorDropdown (any dropdowns hijacking itemToString

Things to look for during review

  • Make sure all references to iot or bx class prefix is using the prefix variable
  • (React) All major areas have a data-testid attribute. New test ids should have test written to ensure they are not changed or removed.
  • UI should be checked in RTL mode to ensure the proper handling of layout and text.
  • All strings should be translatable.
  • The code should pass a11y scans (The storybook a11y knob should show no violations). New components should have a11y test written.
  • Unit test should be written and should have a coverage of 90% or higher in all areas.
  • All components should be passing visual regression test. For new styles or components either a visual regression test should be written for all permutations or the base image updated.
  • Changes or new components should either write new or update existing documentation.
  • PR should link and close out an existing issue

@netlify
Copy link

netlify bot commented Oct 6, 2021

✔️ Deploy Preview for carbon-addons-iot-react ready!

🔨 Explore the source changes: 901434c

🔍 Inspect the deploy log: https://app.netlify.com/sites/carbon-addons-iot-react/deploys/618d66737af86f000a65bdd0

😎 Browse the preview: https://deploy-preview-2932--carbon-addons-iot-react.netlify.app

@netlify
Copy link

netlify bot commented Oct 29, 2021

✔️ Deploy Preview for ai-apps-pal-angular ready!

🔨 Explore the source changes: 901434c

🔍 Inspect the deploy log: https://app.netlify.com/sites/ai-apps-pal-angular/deploys/618d66737af86f000a65bdd4

😎 Browse the preview: https://deploy-preview-2932--ai-apps-pal-angular.netlify.app

<ForwardRef(Warning24) />
</div>
</div>
}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Carbon started using itemToString to add a title to the currently selectedItem in a dropdown; however, this breaks our current implementation of IconDropdowns. I've opened an issue with them to add another prop to allow us to render the selectedItem as an element instead of always calling the itemToString carbon-design-system/carbon#10038

Copy link
Contributor

Choose a reason for hiding this comment

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

FYI. I think we have had that conversation with them before and they didn't want to support that functionality because they didn't agree with the design use case, but let's hope they change their minds.

} else {
handleSelect(item.id, parentId);
}
}}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

carbon seems to have fixed the double-event firing that was happening when clicking the checkbox, so this stopProp isn't needed anymore. You can confirm with:

  1. Go to story path=/story/1-watson-iot-list--with-checkbox-multi-selection
  2. Verify that the multi selection works by clicking on the row, clicking the checkbox and using enter key when row is focused.
  3. Verify the same but with knob isVirtualList checked
  4. Go to story path=/story/1-watson-iot-list--with-checkbox-multi-selection-and-hierarchy
  5. Repeat step 2-3 but also verify that child / parent selections works in BOTH directions including the indeterminate state

@@ -171,7 +171,7 @@ const ImageGalleryModal = ({
secondaryButtonText={modalSecondaryButtonLabelText}
modalHeading={deleteModalTitleText(selectedImage?.id)}
size="xs"
iconDescription={modalCloseIconDescriptionText}
Copy link
Collaborator Author

@kevinsperrine kevinsperrine Nov 8, 2021

Choose a reason for hiding this comment

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

iconDescription is being deprecated in favor of other attributes. On the Modal it's the closeButtonLabel

@kevinsperrine
Copy link
Collaborator Author

There are some issue with PageTitlebar.

Bread crumbs are cut off Sizing on sticky header is messed up
image vs next. image image vs next image
I think something with the breadcrumbs are throwing off multiple stories for PageTitleBar

Good eye. Fixed.

@davidicus
Copy link
Collaborator

davidicus commented Nov 10, 2021

There is an issue with the menu button menu placement

image

There are also some positioning of the dropdown menu items in RTL that are new to this upgrade
image

Copy link
Collaborator

@davidicus davidicus left a comment

Choose a reason for hiding this comment

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

whew! That was a doozy. I think this last one will be it and it should be ok to 🚢

@kodiakhq
Copy link
Contributor

kodiakhq bot commented Nov 11, 2021

This PR currently has a merge conflict. Please resolve this and then re-add the status: ready to merge 🎉 label.

@kodiakhq kodiakhq bot merged commit 143971a into next Nov 11, 2021
@kodiakhq kodiakhq bot deleted the 2929-upgrade-carbon branch November 11, 2021 19:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Repo] upgrade Carbon to 10.47/7.47 [Card][A11Y] Info tooltips remain open when tabbing through
4 participants