sindresorhus / Plash

💦 Make any website your Mac desktop wallpaper
https://sindresorhus.com/plash
MIT License
3.46k stars 128 forks source link

Add support for `[[screenWidth]]` and `[[screenHeight]]` URL variables #56

Closed colejd closed 4 years ago

colejd commented 4 years ago

This PR adds support for [screenWidth] and [screenHeight] placeholders when specifying a URL in the "Open URL..." menu. The values will automatically get swapped out for the actual size.

Closes #6.

colejd commented 4 years ago

I wanted to go with curly brackets, but I went with square brackets since they don't break your URL validation. If you want curly brackets, we need to change things so that Defaults[.url] doesn't hold a URL, but instead something that allows for characters that are invalid in a normal URL. Would that be preferable?

sindresorhus commented 4 years ago

Thanks for working on this. 🙌🏻

I'm fine with using brackets. Can you make it [[screenWidth]], just to ensure it doesn't clash with any normal URLs. [screenWidth] could potentially clash in some search parameters.

This needs to be quickly documented in the readme too.

colejd commented 4 years ago

@sindresorhus Okay, I think I've addressed all of your comments. I've changed it so double-brackets are used; I've also generalized the placeholder code, which should give you some added flexibility in the future.

Please have another look when convenient.

colejd commented 4 years ago

Okay, I think I've addressed everything in this round @sindresorhus. Ready for more review.

colejd commented 4 years ago

Another round of updates for you @sindresorhus.

sindresorhus commented 4 years ago
Screen Shot 2020-09-04 at 14 50 10

I noticed one last thing. In the menu and tooltip here it should show either the decoded URL or the final URL after replacements. I'm leaning towards the final URL. Thoughts?

colejd commented 4 years ago

I think showing the final URL would make sense. Do you know where that value gets populated?

sindresorhus commented 4 years ago

https://github.com/sindresorhus/Plash/blob/1dae24ff235fea0fa606be295b0a204c1dcf601b/Plash/Menus.swift#L5-L29

sindresorhus commented 4 years ago

Can you fix the merge conflict? addInfoMenuItem was moved to a different file in master.

sindresorhus commented 4 years ago

This looks good otherwise.

colejd commented 4 years ago

Fixed!

sindresorhus commented 4 years ago

Thanks for contributing. This is really good 🙌🏻

colejd commented 4 years ago

Thank you!