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 mobile icons - folder path #5607

Merged
merged 1 commit into from
Aug 14, 2024
Merged

Fix mobile icons - folder path #5607

merged 1 commit into from
Aug 14, 2024

Conversation

karandatwani92
Copy link
Contributor

@karandatwani92 karandatwani92 commented Aug 14, 2024

WHY

While writing an article on this feature, I found a BUG.

BEFORE - What was wrong? What was happening before this PR?

if icons were published to a sub-folder, Icon paths were incorrect for the following stubs:

  • browserconfig.stub
  • manifest.stub

HOW

How did you achieve that, in technical terms?

Just prepend a forward slash '/' if publishing to a sub-folder.

Is it a breaking change?

NO

@karandatwani92 karandatwani92 changed the title Fix save to mobile folder path Fix mobile icons - folder path Aug 14, 2024
@pxpm
Copy link
Contributor

pxpm commented Aug 14, 2024

Thanks @karandatwani92 I can see the issue you mentioned.

I think we can have a simpler solution, how about $pathPrefix = Str::start(Str::finish($pathPrefix ?? '', '/'), '/'); on line 35 ? Wouldn't that solve the issue ?

Cheers

@pxpm pxpm assigned karandatwani92 and unassigned pxpm Aug 14, 2024
@pxpm pxpm force-pushed the fix-save-to-mobile-folder-path branch from 9f403b8 to 1c8acf5 Compare August 14, 2024 15:09
@pxpm pxpm merged commit abf351e into main Aug 14, 2024
8 checks passed
@pxpm pxpm deleted the fix-save-to-mobile-folder-path branch August 14, 2024 15:11
@karandatwani92
Copy link
Contributor Author

@pxpm Thanks. I was testing your solution when you just merged. It's working without any issues. Cheers 🍻

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

2 participants