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

Add namespaces support for Colors #35

Closed
bgdn0 opened this issue Jul 4, 2021 · 7 comments
Closed

Add namespaces support for Colors #35

bgdn0 opened this issue Jul 4, 2021 · 7 comments
Assignees
Milestone

Comments

@bgdn0
Copy link

bgdn0 commented Jul 4, 2021

Hello,

Is it possible to add nested namespaces support for generated Colors enum (the same way as for Images enum) ?

@kaandedeoglu
Copy link
Owner

Hey @bgdn0 ,that's a very good suggestion, thanks for getting involved!

I'm a little concerned it'll break code for some people. On the other hand it makes sense to align images & colors. Let's give it a try 🚀

@kaandedeoglu
Copy link
Owner

@bgdn0 So the feature is now working, we also get the nesting behavior for colors (it also uncovered a beautiful refactoring that got rid of a lot of duplicate code). That being said, it now feels a little weird for my projects that use Shark.
I usually tend to put all colors into a Colors folder, therefore after this change we'll go from:

view.backgroundColor = C.primaryRed

to

view.backgroundColor = C.Colors.primaryRed

So I would like to hear about your use cases and whether you organize color assets per folder when building applications.

@bgdn0
Copy link
Author

bgdn0 commented Jul 8, 2021

Hey @kaandedeoglu , cool, when can I expect the feature release to try it out?

We have separate Asset catalogs for Images and Colors.
Colors look something like this (simplified)

Colors.xcassets
	├── button
	│	├── disabled.colorset
	│	├── main.colorset
	│	└── secondary.colorset
	├── input
	│	├── active.colorset
	│	├── error.colorset
	│	├── inactive.colorset
	│	└── placeholder.colorset
	├── list
	│	├── background.colorset
	│	└── separator.colorset
	└── text
		├── error.colorset
		├── main.colorset
		└── secondary.colorset

and the usage

button.backgroundColor = C.button.main
label.textColor = C.text.main

In one project we had colors gray1, ... gray6 and blue1, .... blue4, and it's not obvious should I use gray2 or gray4 for the text, so I prefer to name colors with something more meaningful and use namespaces

@kaandedeoglu
Copy link
Owner

Hey, the changes are now pushed to the namespaced_color_assets branch, you'll have to build from source to test it:

Simply navigating to the root and doing swift build -c release --disable-sandbox should give you an executable at .build/release/Shark. Happy testing 🧀

@bgdn0
Copy link
Author

bgdn0 commented Jul 9, 2021

Works good for me 🎉. Thank you! 👍

If you don't want to break Colors enum compatibility with the previous version, maybe it worth to add use-namespaces-for-colors as a command line option?

@mickeyl
Copy link
Collaborator

mickeyl commented Sep 18, 2022

Hi @bgdn0. When I started working on Shark with regards to the new framework-specific code generation, I was not aware of the namespaced_color_assets branch. I think it's a good idea to bring this into master before working on the next features. I have just rebased it.

While working on this I noticed one gotcha… the nested namespace enum builder does not evaluate the corresponding Xcode setting yet. This can lead to confusion, hence going forward I'd rather evaluate the Xcode Provides Namespace setting than providing yet another command line option:
Bildschirmfoto 2022-09-18 um 14 17 18

I'd also rework the image support then to honor that flag.

What do you think?

@mickeyl mickeyl self-assigned this Sep 18, 2022
@mickeyl mickeyl added this to the 1.6.0 milestone Sep 18, 2022
mickeyl added a commit that referenced this issue Sep 20, 2022
Previously, the nested enum builder did treat every folder as if
[X] Provides Namespace was checked in the Xcode settings.
This patch reads the corresponding `Content.json` files to find out
what the user really intended.
@mickeyl
Copy link
Collaborator

mickeyl commented Sep 23, 2022

This is now life as per the 1.6.0 release.

@mickeyl mickeyl closed this as completed Sep 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants