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

feat(bulk-import): upgrading to mui5 #24

Conversation

its-mitesh-kumar
Copy link
Contributor

@its-mitesh-kumar
Copy link
Contributor Author

cc : @debsmita1

@rm3l
Copy link
Member

rm3l commented Nov 6, 2024

/cc @debsmita1

@openshift-ci openshift-ci bot requested a review from debsmita1 November 6, 2024 17:12
@04kash
Copy link
Collaborator

04kash commented Nov 7, 2024

@its-mitesh-kumar To fix CI issues, you'd have to run yarn dedupe from the bulk-import workspace

@debsmita1
Copy link
Member

Tested the changes locally, and the changes look fine
@its-mitesh-kumar Please fix the Resize observer issue with the side panel

@its-mitesh-kumar
Copy link
Contributor Author

its-mitesh-kumar commented Nov 13, 2024

@divyanshiGupta
Resize observer issue @debsmita1 has mentioned, its a warning that will be shown only in dev mode . It will not be there on prod .
Screenshot 2024-11-13 at 3 41 39 PM
Cc : @christoph-jerolimov

@divyanshiGupta
Copy link

divyanshiGupta commented Nov 13, 2024

If we aren't using ResizeObserver in the code we can maybe ignore this issue for this PR as it will not appear in prod but it will be still better to fix it in the dev mode in this PR if possible and if not then we can merge this PR and raise another for the dev mode once we have a solution.

@its-mitesh-kumar
Copy link
Contributor Author

@divyanshiGupta ! With the below commit resize issue has gone .
69036a1

@christoph-jerolimov
Copy link
Member

I still have the resize issue; it depends on how your OS renders the scrollbar.

The issue is logged here in MUI: mui/material-ui#43718 (there are also multiple duplicates)

For me, this issue appears only when multiple TextFields with the multiline prop (an auto-resizing text field) are used at the same time. But that's exactly what we want here.

I think we can go ahead for now.

Copy link

@divyanshiGupta divyanshiGupta left a comment

Choose a reason for hiding this comment

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

/lgtm

@christoph-jerolimov
Copy link
Member

christoph-jerolimov commented Nov 14, 2024

I noticed one difference, can we check if we can fix this before we merge this?

The background color of the drawer changed in dark mode / theme:

Before:

before

With this PR:

after

@its-mitesh-kumar
Copy link
Contributor Author

@christoph-jerolimov ! For me its looking like below .
Screenshot 2024-11-14 at 4 11 51 PM

@christoph-jerolimov
Copy link
Member

Don't understand how this is possible, but we can start merging and then we will take a look again.

@christoph-jerolimov christoph-jerolimov merged commit 9de1573 into redhat-developer:main Nov 14, 2024
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants