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

Themes cannot be defined as desktop and mobile themes at the same time #7363

Closed
R-J opened this issue Jun 18, 2018 · 2 comments
Closed

Themes cannot be defined as desktop and mobile themes at the same time #7363

R-J opened this issue Jun 18, 2018 · 2 comments

Comments

@R-J
Copy link
Contributor

R-J commented Jun 18, 2018

A Vanilla OS user is creating a responsive theme and would like to make it show up in "normal" themes and mobile themes at the same time (that's the sense of responsive design)

A theme addon has the IsMobile key to determine if it can be used as a mobile theme. But using that key automatically disqualifies it from being used as a desktop theme.


While IsMobile defaults to false, it might be possible to add a IsDektop key which defaults to not IsMobile , which would be true for current desktop themes and false for current mobile themes.

There's no reason why a theme shouldn't be a mobile and a desktop theme at the same time. So a theme could use both keys at the same time.

My first assumption is that only the SettingsController has to be changed when mobile/desktop themes are filtered like that (example for desktop themes):

            // Remove mobile themes, as they have own page.
            if (isset($theme['IsMobile']) && $theme['IsMobile']) {
                unset($themes[$index]);
            }

to

            // Remove mobile only themes, as they have own page.
            if (val('IsMobile', $theme) && !val('IsDesktop', $theme)) {
                unset($themes[$index]);
            }

If you are open to this approach, I would make a PR

@charrondev
Copy link
Contributor

charrondev commented Jun 18, 2018

I'd imagine there will be a different solution to this in 2.8. 1st class support for a responsive theme is on the radar, and the will likely take the form of a value in the addon.json like isResponsive.

For now the simplest solution is to simply enable the theme as both the Garden.Theme and the Garden.MobileTheme at the same time.

This can be automatically done in themehooks. The following would be done for a normal "desktop" theme to be enabled as a responsive theme. This will not alter the which themes show as available in the dashboard though.

class MyThemeHooks extends Gdn_Plugin {
    /**
     * Run once on enable.
     */
    public function setup() {
        saveToConfig([
            'Garden.MobileTheme' => 'my-theme-key'
        ]);
		
		// Do other things in setup...
    }
}

@R-J
Copy link
Contributor Author

R-J commented Jun 18, 2018

Thanks for the extensive information. I was tempted to suggest "isResponsive", too, but I found that too limited. What if there will be another terminology for themes than can be used on desktop and mobile or if "responsive" isn't the right word for such theme.
In the end it should be named after its purpose and not the design pattern of one theme.

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

2 participants