Skip to content

Infra/assets internal #3618

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

Merged
merged 18 commits into from
Mar 20, 2025
Merged

Infra/assets internal #3618

merged 18 commits into from
Mar 20, 2025

Conversation

adids1221
Copy link
Contributor

@adids1221 adids1221 commented Mar 16, 2025

Description

Assets new internal path, moved local components assets to be under Assets.internal.icons.
Component which are part of this PR:
ChipsInput, ColorSwatch, Picker - FieldType, Hint, Wizard, Overaly, ScrollBar, Stepper
This is part of the web effort to support assets.
TODO: add StackAggregator.
Old PR #3602 closed

Changelog

Assets new internal path, moved local components assets to be under Assets.internal.icons.

Additional info

MADS-4531

@Inbal-Tish
Copy link
Collaborator

Inbal-Tish commented Mar 17, 2025

@adids1221 If we're using Assets.internal.icons in our demo screens our users will assume they can use them too. Can we find a solution to mask this usage? (also, we have assets dir in the demo...)

@@ -3933,7 +3933,7 @@ exports[`TextField Screen renders screen 1`] = `
accessible={false}
source={
{
"testUri": "../../../src/assets/icons/xFlat.png",
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we should move the snap tests to the component's dir, they shouldn't be in the demo

Copy link
Collaborator

Choose a reason for hiding this comment

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

But then you'll be referencing the demo from the src code
Also, focus on the assets change, if you decide to do other changes, do it in another PR please

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, we do it already in other snaps and in private as well. I think we better keep tests in src instead of the demo

Copy link
Collaborator

@ethanshar ethanshar left a comment

Choose a reason for hiding this comment

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

@adids1221
I see you use Assets.internal.icons in both src code and demo example screens
IMO, we should it only in the our src code.
For the demo screens (mobile or web) we should demonstrate how the user can load their own assets and use it, we are already doing it, you can find the demo/src/configurations.js file where we configure the demo assets.
Let's use these assets for our example screens instead

@Inbal-Tish @M-i-k-e-l @nitzanyiz

@adids1221
Copy link
Contributor Author

adids1221 commented Mar 17, 2025

Fixed the demo screens Assets usage, changed some file names.

@adids1221 adids1221 added the docs docs related issue label Mar 18, 2025
@Inbal-Tish Inbal-Tish merged commit 6c7f5b3 into master Mar 20, 2025
1 check passed
@Inbal-Tish Inbal-Tish deleted the infra/Assets_internal branch March 20, 2025 10:29
@adids1221 adids1221 restored the infra/Assets_internal branch March 24, 2025 13:59
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.

3 participants