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.

Sign up for GitHub

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

Jump to bottom

Upgrade primereact #347

Merged
merged 13 commits into from
Dec 1, 2022
Merged

Upgrade primereact #347

merged 13 commits into from
Dec 1, 2022

Conversation

mdennis10
Copy link
Contributor

@mdennis10 mdennis10 self-assigned this Nov 25, 2022
@codecov
Copy link

codecov bot commented Nov 25, 2022 •
edited
Loading

Codecov Report

Merging #347 (3201af6) into master (2b77f4b) will not change coverage.
The diff coverage is n/a.

@@          Coverage Diff           @@
##           master    #347   +/-   ##
======================================
  Coverage    99.9%   99.9%           
======================================
  Files          87      87           
  Lines        2505    2505           
  Branches      629     629           
======================================
  Hits         2502    2502           
  Misses          3       3

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Xaes
Xaes suggested changes Nov 25, 2022
View reviewed changes
Copy link
Contributor

@Xaes Xaes left a comment

Choose a reason for hiding this comment

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

Good job, M! I have one question and I also noted a little issue with tooltips and page's overflow. Please, take a look at the video:

Screencast_2022-11-25_16_10_29_AdobeExpress.mp4

It seems this issue only occurs on the first load of the tooltip. If the tooltip has already loaded, it does not creates that little flick. It is also triggered by any other tooltip that's not on the edge.

Also, please check your package-lock.json I think you have not generated it again after your SubUI version bump.

src/subtitleEdit/cues/edit/CueTextEditor.spellcheck.keyboard.shortcuts.spec.tsx Show resolved Hide resolved
@mdennis10
Copy link
Contributor Author

Good job, M! I have one question and I also noted a little issue with tooltips and page's overflow. Please, take a look at the video:

Screencast_2022-11-25_16_10_29_AdobeExpress.mp4
It seems this issue only occurs on the first load of the tooltip. If the tooltip has already loaded, it does not creates that little flick. It is also triggered by any other tooltip that's not on the edge.

Also, please check your package-lock.json I think you have not generated it again after your SubUI version bump.

I'm not able to reproduce the issue outlined in the video locally. I also add package-lock.json can you do a clean 'npm i' and retest if having the same issue

@lkrnac
Copy link
Contributor

lkrnac commented Nov 29, 2022

Please wait with this PR before merging. It seems I introduced bug that was discovered by E2E testing. Looking into it. Will keep you updated.

@Xaes
Copy link
Contributor

Xaes commented Nov 29, 2022

Hey, M! I already did a clean install. Deleted node_modules and everytyhing. The issue still persists. I even tried to package it into VTMS and the bug is present there as well. This is the environment I am using:

Screenshot from 2022-11-29 11-15-49

Screenshot from 2022-11-29 11-17-12

Let me know if you need anything else to reproduce this issue.

@Xaes
Copy link
Contributor

Xaes commented Nov 29, 2022

Did a quick Google search, found this: primefaces/primereact#3687 . Seems like we have to upgrade to the next minor version or implement the CSS code provided by the project's mantainer in the issue's comments.

@adolfov adolfov mentioned this pull request Nov 29, 2022
Merged
@mdennis10
Add fix for tooltip
ee8076d
@mdennis10
Merge branch 'master' into mario-VTMS-4128-upgrade-primereact
f83de7b 
# Conflicts:
#	package-lock.json
@mdennis10
Update package.json
c5cb947
@mdennis10
Merge remote-tracking branch 'origin/mario-VTMS-4128-upgrade-primerea…
49b54fb 
…ct' into mario-VTMS-4128-upgrade-primereact

# Conflicts:
#	package-lock.json
#	package.json
@mdennis10
Copy link
Contributor Author

Ready for review

@Xaes Xaes self-requested a review November 29, 2022 23:00
Xaes
Xaes approved these changes Nov 29, 2022
View reviewed changes
Copy link
Contributor

@Xaes Xaes left a comment

Choose a reason for hiding this comment

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

Good work, M! Works as expected with the fix. Approving, assumming that you will take care of my single comment. I also tested this with VTMS, and looks good.

src/global.css Outdated
@@ -652,3 +652,13 @@ region.wavesurfer-region:before {
content: attr(data-region-label);
@apply text-gray-900/80
}

/* TODO - Fixes tooltip flicker issue introduced
Copy link
Contributor

Choose a reason for hiding this comment

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

Please, lookup for a tooltip section in this very same CSS file and put this piece of code there, to keep things into one same place.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@mdennis10 mdennis10 merged commit 8869dae into master Dec 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Xaes Xaes Xaes approved these changes

@lkrnac lkrnac Awaiting requested review from lkrnac

@Youans Youans Awaiting requested review from Youans

@fbnt fbnt Awaiting requested review from fbnt

@adolfov adolfov Awaiting requested review from adolfov

@rpoet-ds rpoet-ds Awaiting requested review from rpoet-ds

Assignees

@mdennis10 mdennis10

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@mdennis10 @lkrnac @Xaes