mcongrove / ChariTi

Please do not use: this is out-dated
Other
113 stars 85 forks source link

Solution to Issue #160: Slide Menu Section Headers #184

Closed Aaron-Hartwig closed 10 years ago

Aaron-Hartwig commented 10 years ago

This is my proposed solution to theissue/feature request of adding support for menu headers in the slide menu. I attached a photo of this in use in a current ChariTi application:

menu header example

mcongrove commented 10 years ago

I'm a little confused; it seems you have some duplicated data in the JSON config file:

https://github.com/Aaron-Hartwig/ChariTi/commit/bbfe30cc4d4c3cf64f8e417f9ab08489b80e99ea#diff-ff774ac5fd087b85c80be3027b047b94R32

Is it supposed to be like that, or should it just have a header title and 'is header' flag?

Aaron-Hartwig commented 10 years ago

My apologies. Actually all it needs is a title, a type (just set to "text" so it does throw an error on line 269 of core), and the "is header" flag.

mcongrove commented 10 years ago

After looking at this a bit more, I wonder if there's a slightly more elegant way to go about completing this objective. The part that sort of annoys me is the 'type: text' requirement, which if left out throws an error.

What if, instead of having a separate object in the 'tabs' object of the JSON file like this:

... {
    title: "My Header",
    menuHeader: true
}, ...

We instead just tacked the header title onto the tab we wish it to be on top of, like this:

... {
    menuHeader: "My Tabs",
    title: "Home",
    type: "text",
    text: "Lorem ipsum..."
}, ...

Or at the very least, we need to come up with a better way than having to keep the 'type: text' in there to keep it from throwing that error. It also might cause other problems, because the code thinks the menu header is actually a tab item... it even gets a tab ID, which it probably shouldn't.

Thoughts?

Aaron-Hartwig commented 10 years ago

I like that idea quite a bit. If there is a menu header (it's value can just be it's title) then the tab it is attached to would remain the same but with a header on top of it. So would we want the header background color to be set to the apps secondary color perhaps?

mcongrove commented 10 years ago

We should set it to the primary color; if you take a look at the 1.2 branch that's been updated to styles more closely resembling iOS 7, you'll see the menu is black. Using the primary color as the background, with white text, should be fine. Just make sure it uses the new font styling and all, please.

I guess the property should be headerTitle?

Aaron-Hartwig commented 10 years ago

Yea that would make sense. I'll hopefully be able to get at this later tonight.

mcongrove commented 10 years ago

Awesome; just submit a new pull request and I'll get on it ASAP. I'm going to close this one out without merging just to keep things clean.