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: update UI5 from v1 to v2 #3542

Open
wants to merge 37 commits into
base: main
Choose a base branch
from
Open

chore: update UI5 from v1 to v2 #3542

wants to merge 37 commits into from

Conversation

mrCherry97
Copy link
Contributor

@mrCherry97 mrCherry97 commented Dec 17, 2024

Description

Changes proposed in this pull request:

  • updated all UI5 dependencies to V2
  • updated React to version 18
  • refactored components to the UI5 v2 API
  • adjusted e2e tests
  • migrated unit and component tests from Vitest to Cypress
  • replaced spacing from webcomponents-react with corresponding classNames
  • fixed bugs

Related issue(s)
Closes #3422

Definition of done

  • The PR's title starts with one of the following prefixes:
    • feat: A new feature
    • fix: A bug fix
    • docs: Documentation only changes
    • refactor: A code change that neither fixes a bug nor adds a feature
    • test: Adding tests
    • chore: Maintainance changes to the build process or auxiliary tools, libraries, workflows, etc.
  • Related issues are linked. To link internal trackers, use the issue IDs like backlog#4567
  • Explain clearly why you created the PR and what changes it introduces
  • All necessary steps are delivered, for example, tests, documentation, merging

OliwiaGowor and others added 22 commits October 31, 2024 13:44
* chore: update packages

* chore: bump ui5-react

* fix: adjust tests to not use enzyme

* chore: fix dependencies

* fix: remove unused

* fix: remove not existing import

* chore: add compatibility package
* fix: change render

* fix: adjust css names

* fix: change imports for Toolbar

* fix: change imports for tables

* fix: rename list comp. & badge

* fix: fix imports
* change some spacing to new classnames

* fix merge

* pray to god, that everything is working

* pray to god, that everything is working

* some spacing and iconEnd for ExternalLink

* some spacing and iconEnd for SA token

* add no-margin class in index.scss

* adjust the rest of the spacing

* remove unused impott

* fix stupid classNames

* adjust separatorLine to tsx and use style prop

* review changes
* fix: ui5 codemode adjustments

* fix: fix imports

* fix: fix table imports

* fix: fix unused
* fix: adjust dynamicPage

* fix: separate actionsBar

* fix: fix actions behavior

* fix: remove unused

* fix: fix classNames

* fix: fix empty toolbar & move banners

* fix: fix dependency

* fix: remove spacing file
* fix: adjust tokens

* fix: adjust spacing
* fix: adjust badges type names

* fix: fix type mappings

* fix: remove console.log
* fix: add 'size' to headers

* fix: add size to headers
* remove defaultProps and disableEdit prop from resourcelist

* Update src/shared/components/ResourcesList/ResourcesList.js

Co-authored-by: Oliwia Gowor <72342415+OliwiaGowor@users.noreply.github.com>

---------

Co-authored-by: Oliwia Gowor <72342415+OliwiaGowor@users.noreply.github.com>
* fix: fix styling and alerts

* fix: fix unsaved message

* fix: fix background in add modules

* fix: remove unused

* fix: passing props

* fix: key error

* fix: resources in list
* fix: adjust Preferences modal

* fix: move import

* fix: opening preferences menu

* fix: list props
* fix: key errors

* fix: fetch error

* fix: monaco worker fix

* fix: dynamicPage header error

* fix: small fixes

* fix: review fixes
* fix: adjust tables

* fix: emptyListComp flickering

* fix: adjust reset.css
* fix: fix flickering on column layout

* fix: remove unused

* fix: replace DynamicPage with ObjectPage

* fix: fix key error

* fix: banner height and remove unused

* fix: initial render flicker

* fix: change to DynamicPage

* hotfix

* Replace props spread

* hotfix

* fix: shadow & unused

* fix: shadow & sticky tabs

* fix: hide button everywhere

---------

Co-authored-by: mrCherry97 <mwisnia97@gmail.com>
Co-authored-by: akucharska <agata.kucharska@sap.com>
@kyma-bot kyma-bot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Dec 17, 2024
@mrCherry97 mrCherry97 changed the title Ui5 migrate v2 chore: update UI5 from v1 to v2 Dec 17, 2024
@kyma-bot kyma-bot added the cla: yes Indicates the PR's author has signed the CLA. label Dec 17, 2024
mrCherry97 and others added 3 commits December 17, 2024 12:46
Co-authored-by: Oliwia Gowor <oliwiagowor@gmail.com>
* Fix delete cluster action
* Revert "Merge branch 'main' into ui5-migrate-v2"

This reverts commit 79fb867, reversing
changes made to 5b7fd61.

* fix: selected in lists (#3539)

* fix: toasts in dialogs
mrCherry97 and others added 6 commits December 20, 2024 14:06
* fix: adjust tests

* fix: adjust more tests

* fix: selected in list

* comment out rebase action

* comment out rebase action v2

* fix: create namespace

* fix: adjust comboboxes

* fix: adjust tests

* fix: add wait

* cleanup
* fix: small fixes

* fix: ProgressIndicator colors

* fix: sidebar z-index

* fix: acc tests

* fix: more acc
@OliwiaGowor OliwiaGowor marked this pull request as ready for review December 27, 2024 08:40
@kyma-bot kyma-bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Dec 27, 2024
* cypress config for component testing

* first tests migration

* migrated badge tests

* migrated generic list

* migrated counting card

* migrated copiable text

* migrated namespace settings

* migrated ui5radial chart

* migrated podrestarts

* migrated errorboundary

* migrated pagination

* migrated listactions

* migrated resourcenotfound

* migrated modalwithform

* migrated helmreleases

* migrated selector

* migrated ResourceLink

* migrated controlledby

* mocked ui5 dependencies in nodeQueries test

* migrated JoinedArray

* migrated CodeViewer

* mocked ui5 dependencies in useCreateResource test

* mocked ui5 dependencies in useGetCRDByPath

* migrated Columns

* migrated Widget

* mocked ui5 dependencies in linkExtractor

* migrated SecretData

* migrated SideDrawer

* migrated ResourceDetailsColumns

* migrated ResourceDetailsVisibility

* fixed issues in tests

* migrated Table

* mocked ui5 dependencies in Widget.copyable

* fixed useGet test

* refined vitest

* adjust github actions workflow

* adjust github actions workflow

* added command for headless component tests

* add cypress as dev dependency

* fixed genericList test

* adjusted testing-strategy.md

* Update docs/contributor/testing-strategy.md

Co-authored-by: Iwona Langer <iwona.langer@sap.com>

---------

Co-authored-by: Oliwia Gowor <72342415+OliwiaGowor@users.noreply.github.com>
Co-authored-by: Iwona Langer <iwona.langer@sap.com>
@OliwiaGowor OliwiaGowor requested a review from a team as a code owner December 30, 2024 10:22
Copy link
Contributor Author

@mrCherry97 mrCherry97 left a comment

Choose a reason for hiding this comment

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

Part of the review (253/307 files checked). Please take a look:

  • CommandPallete shouldn't be able to navigate when Create/Edit modal is filled )please also open an issue to add this to test cases or add it)
Screen.Recording.2025-01-03.at.09.32.02.mov
  • Something is wrong with Edit form (mostly on Modules view), when I make some changes, and wait for like 5-6 seconds, modal is refreshed to initial state, I can click Add, but there are no changes
Screen.Recording.2025-01-03.at.09.55.28.mov
  • Presets are not working, clicking arrow is marking Name label, and I can't write anything (we should have to choose some preset in the tests)
Screen.Recording.2025-01-03.at.10.22.14.mov

Comment on lines -19 to -24
- name: Install Chrome # this step could be removed after https://github.com/cypress-io/cypress/issues/30374 is resolved
shell: bash
run: |
wget --no-verbose -O /tmp/chrome.deb https://dl.google.com/linux/chrome/deb/pool/main/g/google-chrome-stable/google-chrome-stable_127.0.6533.119-1_amd64.deb
sudo apt install --allow-downgrades -y /tmp/chrome.deb
rm /tmp/chrome.deb
Copy link
Contributor Author

Choose a reason for hiding this comment

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

we can remove it, but it will be nice to have it documented in internal docs, how to do this, in case same problem will appear in the future

@@ -21,7 +21,7 @@
<meta name="theme-color" content="#000000" />
<meta
http-equiv="Content-Security-Policy"
content="font-src 'self' https://sdk.openui5.org/; script-src 'self' blob:; object-src 'self';"
content="font-src 'self' https://sdk.openui5.org/; script-src 'self' 'unsafe-eval' blob:; object-src 'self';"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this change should be also on the main, please double-check if there is no problem between the main and feature-branch

- 'src/**'
- 'backend/**'
- 'package.json'
- 'cypress/support/**'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

To consider, in my opinion we need to run this workflow every time when something will be changed in whole cypress.

Suggested change
- 'cypress/support/**'
- 'cypress/**'

...spacing.sapUiMediumMarginBegin,
...spacing.sapUiMediumMarginTopBottom,
}}
size="H3"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

is level prop necessary? size is impacting styles, looks like level is not making anything

Copy link
Contributor

Choose a reason for hiding this comment

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

The level prop defines the header level, so it is mainly used for accessibility purposes

...spacing.sapUiMediumMarginBegin,
...spacing.sapUiMediumMarginTopBottom,
}}
size="H3"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same question for all of the Title components
is level prop necessary? size is impacting styles, looks like level is not making anything

@@ -27,7 +27,7 @@ export function ModeSelector({ mode, setMode, isDisabled = false }) {
<SegmentedButton
className="mode-selector__content"
onSelectionChange={event => {
const mode = event.detail.selectedItem.getAttribute('data-mode');
const mode = event.detail.selectedItems[0].getAttribute('data-mode');
Copy link
Contributor Author

Choose a reason for hiding this comment

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

are you sure, that hardcoded [0] will be working every time without error?

Copy link
Contributor Author

@mrCherry97 mrCherry97 left a comment

Choose a reason for hiding this comment

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

Finish of my review.

  • Something is wrong with dropdowns/options (Check APIRules service dropdown)
    image

Also after choosing some of the option, empty option is displayed
image

Last option is most of the time empty (API Rules -> Access Strategy):
image

  • Check styles of the header in Modules view
    New:
    image

Old:
image

navigated={isSelected}
selected={isSelected}
>
<TableRow type="Active" selected={isSelected}>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just question, navigated prop should be removed?

@@ -24,7 +24,7 @@
min-width: 200px;
}

@container (min-width: 650px) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Search for the rest of @container and check if it's working, there is 16 results in 10 files

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes Indicates the PR's author has signed the CLA. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Migrate ui5-webcomponents-react from v1 to v2
8 participants