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

Rename Skin abstractions to Theme #1

Open
wants to merge 1 commit into
base: feature/skin-service
Choose a base branch
from

Conversation

bdukes
Copy link

@bdukes bdukes commented Nov 3, 2023

This is a potential update to dnnsoftware#5838 to adjust the names of the abstractions to use Theme instead of Skin.

@bdukes
Copy link
Author

bdukes commented Nov 3, 2023

@mitchelsellers
Copy link

I'm really torn on this, as even if we make the change here is is only a partial change as the actual Themes are still stored in the /Skins folder within the portal Or root.

@valadas
Copy link

valadas commented Nov 3, 2023

I don't have a strong opinion on this but leaning towards leaving it at Skins just for consistency at least for developers. To keep it consistent then everything with the words Skin would have to become Theme, so ThemeObjects, folder names, manifests, etc. and the would be massive breaking change. Sooo, I am leaning towards leaving Skin terminology in the backend myself.

@david-poindexter
Copy link

Well, we are already "straddling the fence" on these two terms, so I would be a fan of just making things consistent one way or the other. Skin is what it has always technically been. Theme is better for outsiders coming in, but Corp didn't go "all in" when they started using Theme. Documentation is currently Theme but the code unfortunately doesn't match. So...is there a chance we could go "all in" one way or the other? For what it is worth, I like Theme but this would of course mean having to change folder names, etc.

Furthermore, I honestly think the Skin(s) approach was flawed semantically from the beginning. We had no clear distinction between a skin "package" and skin "layout". So I think theme and layout clear this up semantically. Container is fine regardless.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants