Skip to content

Condense main menu + add skill categories#373

Open
tylxr59 wants to merge 5 commits intoStypox:masterfrom
tylxr59:condense-main-menu
Open

Condense main menu + add skill categories#373
tylxr59 wants to merge 5 commits intoStypox:masterfrom
tylxr59:condense-main-menu

Conversation

@tylxr59
Copy link
Contributor

@tylxr59 tylxr59 commented Dec 14, 2025

Made WhatICanDo.kt collapsible on main app screen.

Added categories for skills. These are used to categorize skills in the What I Can Do card and Skill Settings. Skill names are translatable via strings.xml and uses a collator to properly alphabetize categories based on the localized category names. Skills lacking a category are handled by dropping them into the Other category.

Categories are assigned in the SkillInfo.kt files. Example from current_time:

object CurrentTimeInfo : SkillInfo("current_time") {
    override val categoryNameRes = R.string.category_information

Copy link
Owner

@Stypox Stypox left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the nice change! I left some minor comments, but I'm very in favor of categorization.

Btw, I remember you posted some screenshots of this in Matrix; maybe also post them in the PR body next time, so visitors of the PR will see what it is up to at a glance :-).

val collator = Collator.getInstance(locale)

// Group skills by category and sort
val categorizedSkills = skills
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This calculation should go in the viewModel, otherwise it gets recomputed on every recomposition

val enabledSkills by viewModel.enabledSkills.collectAsState()

// Get current locale for proper collation
val locale = context.resources.configuration.locales.get(0) ?: Locale.getDefault()
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is theoretically fine, but for the Locale I'd prefer if you used LocaleManager.locale.value in the viewModel

* category. If not overridden, skills will be placed in the "Other" category.
* @return the string resource ID for the skill's category name (e.g. R.string.category_productivity)
*/
open val categoryNameRes: Int = 0 // Will default to R.string.category_other in app module
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of using a string resource, create an enum with all categories. The enum constructor then would take the string resource (and an icon?). This keeps the UI distinct from the fields of SkillInfo, and also makes it clear at a glance which categories exist.

Btw, generally, when you have Res ints, always add @StringRes.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was on purpose ;-)

val collator = Collator.getInstance(locale)

// Group skills by category and sort
val categorizedSkills = skills
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This code is duplicate with the above, it should be deduplicated somehow. And again it should not go in the UI, but here you can remember {} it since we don't have a view model.

Text(
text = stringResource(R.string.here_is_what_i_can_do),
style = MaterialTheme.typography.bodyMedium,
color = MaterialTheme.colorScheme.onSecondaryContainer.copy(alpha = 0.7f),
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd keep the color as-is, since here you are basically introducing a new color to the palette; are they not distinct enough without?

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.

2 participants