Skip to content

Conversation

MiroslavDionisiev
Copy link
Contributor

@MiroslavDionisiev MiroslavDionisiev commented Mar 13, 2025

Resolves:

UEPR-57

Changes:

  • Updated react to v18 and other related packages. For the packages that are also in scratch-gui the version is used. Updated studios routing. The previously used testing framework - Enzyme, won't be supported for react-18 and tests are migrated to React Testing Library (RTL). Most of the component tests rely on asserting props and state of the component, however RTL doesn't allow such assertions as what is intended to be tested by the library is only what the user would interact with. To bypass this there is a wrapper around the render method from RTL that searches the structure of the React element for the node that contains the props and state. We should discuss the approach before further updating the rest of the tests.
  • Added script to generate translations before executing tests.
  • Removed use of DefaultProps that will be deprecated in future releases for functional components.

@MiroslavDionisiev MiroslavDionisiev marked this pull request as ready for review April 22, 2025 06:38
Copy link
Contributor

@cwillisf cwillisf left a comment

Choose a reason for hiding this comment

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

I picked a nit or two, but it's low-priority stuff. Kudos for getting through this!
👏👏👏

Comment on lines 117 to 118
# run unit tests in development mode to address error "act(...) is not supported in production builds of React"
NODE_ENV=development JEST_JUNIT_OUTPUT_NAME=unit-jest-results.xml npm run test:unit:jest:unit -- --reporters=jest-junit
Copy link
Contributor

Choose a reason for hiding this comment

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

If test:unit:jest:unit doesn't work outside of dev mode, would it make sense to move the NODE_ENV=development part into package.json? Something like:

    "test:unit:jest:unit": "cross-env NODE_ENV=development jest ..."

const Avatar = props => (
const Avatar = ({
className,
src = '//uploads.scratch.mit.edu/get_image/user/2584924_24x24.png?v=1438702210.96',
Copy link
Contributor

Choose a reason for hiding this comment

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

...lol. Thanks, Ray.

);
}
for (let i = 0; i < items.length; i++) {
const thumbnails = items[i].thumbnails.map((item, j) => (
Copy link
Contributor

Choose a reason for hiding this comment

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

😎

const SocialMessage = props => (
<props.as className={classNames('social-message', props.className)}>
const SocialMessage = ({
as: Tag = 'li',
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

Comment on lines 16 to 22
messages = {
'teacherbanner.greeting': 'Hi',
'teacherbanner.subgreeting': 'Teacher Account',
'teacherbanner.classesButton': 'My Classes',
'teacherbanner.resourcesButton': 'Educator Resources',
'teacherbanner.faqButton': 'Teacher Account FAQ'
},
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you know if this complicates message scraping? I don't expect this change to make it worse, but it made me curious.

Copy link
Contributor

Choose a reason for hiding this comment

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

Compared to the previous state or to something else? Speaking without full certainty, but I believe this change should lead to the same bundle outcome as the previous one - and these default values should be visible to scrapers.

Comment on lines 24 to 25
expect(donateText.textContent).toEqual(
'Scratch is a nonprofit that relies on donations to keep our platform free for all kids. Your gift of $5 will make a difference.'
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe a snapshot would be better?

.then(response => {
expect(mockedValidateEmailRemotely).toHaveBeenCalledTimes(1);
expect(response.valid).toBe(false);
expect(response.errMsgId).toBe('general.error');
})
.then(() => {
// make the same request a second time
instance.validateEmailRemotelyWithCache('[email protected]')
emailStepInstance.validateEmailRemotelyWithCache('[email protected]')
.then(response => {
expect(mockedValidateEmailRemotely).toHaveBeenCalledTimes(2);
expect(response.valid).toBe(false);
Copy link
Contributor

Choose a reason for hiding this comment

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

Phew! That file was a lot ;)


useEffect(() => {
if (hasRendered.current) {
throw new Error('This component has been re-rendered!');
Copy link
Contributor

Choose a reason for hiding this comment

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

😎

@@ -21,6 +21,7 @@ ENV

# Test
/test/results/*
/test/generated/generated-locales.js
Copy link
Contributor

Choose a reason for hiding this comment

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

Props for remembering to add this line!

@@ -52,12 +63,19 @@ const Grid = props => {
<Thumbnail
href={href}
key={key}
loves={item.stats.loves}
Copy link
Contributor

Choose a reason for hiding this comment

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

Were those added on purpose?

StudioDescription.handleClickOutside = () => {
const ref = useRef(null);

useOnClickOutside(ref, () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Did we test this?

const base = `/${studioPath}/${studioId}`;
const classes = useCallback(({isActive}) => `nav-link ${isActive ? 'activated' : ''}`);
Copy link
Contributor

@KManolov3 KManolov3 Aug 26, 2025

Choose a reason for hiding this comment

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

Should these be nav_link and active ?

@KManolov3 KManolov3 requested a review from cwillisf September 2, 2025 13:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants