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

fix(ADA-1359): Make scrollbar visible all the time #65

Conversation

valentinStoicaK
Copy link
Contributor

@valentinStoicaK valentinStoicaK commented Aug 9, 2024

Root cause:

  • Scrollbar was not visible;

PR description:

  • I've modified the theme.css so that the scrollbar was similar with the one in the transcript plugin;
  • I've removed scroll handling from the playlist plugin;
  • I've removed a comma from playlist-wrapper.css, because of the errors that were created by it;

PS: I don't have the possibility of testing it on Safari and QA informed me that the scrollbar was not visible at all on safari before the fix;

@Tzipi-kaltura Tzipi-kaltura changed the title ADA-1359 - Make scrollbar visible all the time fix(ADA-1359): Make scrollbar visible all the time Aug 12, 2024
}
}, []);

const handleFocus = useCallback(() => {}, []);
Copy link
Contributor

Choose a reason for hiding this comment

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

if that's only noop methods, why keep them? Can we remove handleFocus, handleBlur and handleKeyDown methods?

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

@SivanA-Kaltura
Copy link
Contributor

@valentinStoicaK please change the version of cypress in package.json to 13.13.1, run yarn install, and commit package.json and yarn.lock. this should solve the firefox timeout that's failing the tests.

@valentinStoicaK
Copy link
Contributor Author

@SivanA-Kaltura Done!

@valentinStoicaK valentinStoicaK merged commit 8dea3f2 into master Sep 11, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

3 participants