Skip to content

Feat: [UEPR-165] merge native into develop with platform #220

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

Open
wants to merge 122 commits into
base: develop
Choose a base branch
from

Conversation

Bogomil-Stoyanov
Copy link
Contributor

@Bogomil-Stoyanov Bogomil-Stoyanov commented Mar 18, 2025

Ticket:
UEPR-165

paulkaplan and others added 30 commits April 19, 2019 12:56
Use the same implementation as onSetProjectThumbnailer which does the same thing for getting a thumbnail externally.
Usually we generate a thumbnail when a project is changed. If a project is imported on Android it doesn’t have a thumbnail, and if you just run it without changes it still doesn’t have a thumbnail.

Adding `saveThumbnailOnLoad` flag that Android sets if the project doesn’t have a thumbnail.
Generate a thumbnail for new projects and imported projects when loaded on android
* use `isShowingWithId` instead of `isLoading`, loading finishes while project id is still `“0”` for new projects
* use a timeout for storing the project thumbnail to ensure that it happens after target skins are rendered (they are rendered async)
@Bogomil-Stoyanov Bogomil-Stoyanov marked this pull request as ready for review March 20, 2025 08:20
@@ -0,0 +1,219 @@
name: CI/CD

Copy link
Contributor

Choose a reason for hiding this comment

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

Is this used for anything? Can it be removed?

@@ -196,6 +196,8 @@ describe('Working with costumes', () => {
await expect(logs).toEqual([]);
});

// Doesn't work on Android
/*
// TODO: This sometimes results in a stack overflow error in the GitHub Actions workflow - flaky test.
// Seems like at random another costume is shown. Need to debug
test.skip('Costumes animate on mouseover', async () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's either rewrite or delete this

const loadSpanish = () =>
import(/* webpackChunkName: "es-steps" */ './es-steps.js')
.then(({esImages: imageData}) => imageData);
// const loadSpanish = () =>
Copy link
Contributor

@KManolov3 KManolov3 Mar 20, 2025

Choose a reason for hiding this comment

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

Let's uncomment this. Where is it used? Does it result in missing files on android?

@Bogomil-Stoyanov Bogomil-Stoyanov force-pushed the feat/uepr-165-merge-native-into-develop-with-platform branch from 8f03083 to bd723db Compare March 25, 2025 11:31
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.

Wow, I shouldn't have said that the Android PR was surprisingly simple... this one makes up for it! ;)

I had very little to add on top of Kalo's existing comments. Great work! I'm sure we'll be able to simplify things in the future, maybe merge some code paths, but getting the paths next to each other in the same branch is a huge step. Thank you!

@@ -24,8 +24,23 @@ const PHASES = keyMirror({
updatePeripheral: null
});

const ConnectionModalComponent = props => (
<Modal
const ConnectionModalComponent = props => {
Copy link
Contributor

Choose a reason for hiding this comment

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

@MarshTheTester, we'll need to specifically check hardware extensions for the next Android release. We should include both new (CDM) and old (non-CDM) tablets.

@@ -0,0 +1,146 @@
import PropTypes from 'prop-types';
Copy link
Contributor

Choose a reason for hiding this comment

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

Do #140 and #220 use the same commits for ScratchImage? Just nervous about trying to merge them both to develop at some point 😅

Copy link
Contributor

Choose a reason for hiding this comment

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

Conflicts should be resolved in #226

@KManolov3
Copy link
Contributor

KManolov3 commented Apr 7, 2025

Thinking of closing this and migrating further discussion to - #226 - mainly because some comments/issues here are addressed in that PR and it doesn't make sense for this to be merged on its own anymore. I could add a link for (some) backtracing.
Alternatively, I could keep this open with the idea that we'll merge the two PRs consequently.

What do you think @cwillisf

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.

7 participants