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

BUGFIX: Always bundle custom svgs instead of using resource:// paths #3836

Merged
merged 3 commits into from
Sep 2, 2024
Merged

BUGFIX: Always bundle custom svgs instead of using resource:// paths #3836

merged 3 commits into from
Sep 2, 2024

Conversation

mhsdesign
Copy link
Member

@mhsdesign mhsdesign commented Aug 8, 2024 •
edited
Loading

Resolves: #3616
Resolves: #3712

See #2092 (comment) for an explanation

What I did

The custom icons are now included into the bundle instead of requested by path.

image

Also to unify things and simplify the build-stack, the Neos icon is now also bundled as plain svg instead of data-url:

image

Also the fallback image of the image editor will be fixed for cloud hosted resources now:

before after
image image

Surprisingly i could not observer any pixel shifting. So this should be 100% the same visual look.

How I did it

How to verify it

@github-actions github-actions bot added Bug Label to mark the change as bugfix 8.3 labels Aug 8, 2024
@mhsdesign mhsdesign mentioned this pull request Aug 8, 2024
Closed
Sebobo
Sebobo requested changes Aug 9, 2024
View reviewed changes
Copy link
Member

@Sebobo Sebobo left a comment

Choose a reason for hiding this comment

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

Works and looks great, but I don't see yet how we can deprecate the now old ResourceIcon if the new one does something else and from what I see doesn't support custom icons.

packages/react-ui-components/src/Icon/resourceIcon.tsx Show resolved Hide resolved
@mhsdesign
BUGFIX: Include the image editors dummy image in the bundle
dc6cfbc 
using `/_Resources/Static/Packages/Neos.Neos/Images/dummy-image.svg` is a bad bet as for cloud hosted resources this wont work and it will show a dead-fallback image.
@mhsdesign
Copy link
Member Author

As written in #2092

Case (3.) is more complicated, I'm afraid. The <ResourceIcon/> is a specialization of the (unfortunately public API) <Icon/>-component. We will need to deprecate its behavior, and (prior to that) find a way to allow for custom Icon URIs that can be resolved on the server side (we'll some sort of configurable mapping thing here).

My idea is to deprecate the behaviour but thats just for us and without real effect obviously.
To get rid of all usages we will - and i will plan do so - make the NodeTypeSchemaBuilder transform all resource:// paths on the server.

That will leave us with no more usages of client resource:// paths. Wdyt?

Sebobo
Sebobo approved these changes Aug 12, 2024
View reviewed changes
Copy link
Member

@Sebobo Sebobo left a comment

Choose a reason for hiding this comment

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

Works on my machine <3

@mhsdesign mhsdesign mentioned this pull request Aug 12, 2024
Open
mhsdesign
mhsdesign commented Aug 12, 2024
View reviewed changes
packages/neos-ui-editors/src/Editors/Image/resource/dummy-image.dataurl.svg Outdated
Copy link
Member Author

Choose a reason for hiding this comment

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

This is now duplicated here and in Neos.Neos but yeah there is no way around this ... and it somehow belongs here imo

@mhsdesign
TASK: Optimize Neos logo.svg
5b32b0d 
see discussion in Neos.Neos where we did the same https://github.com/neos/neos-development-collection/pull/5126/files#r1625874465

- remove obsolete `g` tags (and german illustrator references :D)
- inline style
grebaldi
grebaldi approved these changes Aug 28, 2024
View reviewed changes
Copy link
Contributor

@grebaldi grebaldi left a comment

Choose a reason for hiding this comment

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

Looks & works very well :)

@mhsdesign mhsdesign merged commit acffbc3 into 8.3 Sep 2, 2024
10 checks passed
@mhsdesign mhsdesign deleted the bugfix/3616-bundle-custom-icons-insteadof-linking branch September 2, 2024 11:55
mhsdesign added a commit that referenced this pull request Sep 19, 2024
@mhsdesign
BUGFIX: Neos logo icon width
6a65d87 
Regression from #3836
@mhsdesign mhsdesign mentioned this pull request Sep 19, 2024
Merged
mhsdesign added a commit to mhsdesign/neos-ui that referenced this pull request Oct 9, 2024
@mhsdesign
TASK: Dont use react inside terminateDueToFatalInitializationError
130e7d8 
Fixes faulty upmerge of neos#3836
@mhsdesign mhsdesign mentioned this pull request Oct 9, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@grebaldi grebaldi grebaldi approved these changes

@Sebobo Sebobo Sebobo approved these changes

@markusguenther markusguenther Awaiting requested review from markusguenther

Assignees
No one assigned
Labels
8.3 Bug Label to mark the change as bugfix
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@mhsdesign @Sebobo @grebaldi