New: Adapt buttons overhaul (fixes #469) #490

guywillis · 2024-01-25T15:55:34Z

Fixes: #469

New

Expanded on _colors.less variables based on the categories and states defined in ( Adapt Buttons - Vanilla issue #469 (comment))
Identified missing categories / states and updated as required.
Created mixins to normalise button styles based on the catergories and states defined. Default button style set to 'filled' to mimic current Vanilla and 'outline' as optional style. The implementation allows for additional styles to be created for custom themes.
Added UI btn related margins (nav, notify, drawer).
Replace hardcoded UI btn border-radius with variable (fixes Replace hardcoded icon button border-radius with variable #451)
Added focus states through out.
Notify strapline simplification.
Updated properties.schema and theme.schema to reflect latest _colors.less variables.

Note

The following has been implemented to prevent any breaking changes:

Deprecated color variables left in for legacy support only (with comments). All new color variables inherit from existing color variables for backwards compatibilty.
Deprecated .notify-btn-icon mixin left in for legacy support only (with comments). New .ui-btn-ctrl provided as fallback for backwards compatibilty.
New color mixins applied to deprecated .btn-text and .btn-icon selectors for legacy support only (with comments).

Summary

For access to edit the new color variables in the AAT, properties.schema and theme.schema have been updated to reflect the latest _colors.less variables.
For access to edit the button styles in the AAT (e.g set an 'outline' button style), there's an outstanding PR awaiting work New: Added theme variables to compilation (fixes #3577) adapt_framework#3578.

@visited

- focus state inherits from hover (as per existing component focus styles) - existing @visited vars are only used by components so grouped with content item

- focus state inherits from hover (as per existing focus styles) - locked state inherits from disabled (as per existing locked styles)

- Replace @btn-ui-color vars with @ui-color vars. Nav, Notify and Drawer btns inherit from their view colours. This sets an extra level of vars to define global ui colors or edit these separately as per existing Vanilla. - rename _buttonStates.less file to _state-mixins.less.

- ui-btn-controls vars added - shared Notify and Drawer btn styles combined into single ui-btn--controls mixin. Replacing .notify-btn-icon mixin.

- set menu item btn vars to inherit from item ui

- item ui locked state added - set .narrative__strapline-icon to inherit from narrative__strapline-btn to reduce additional state declaration styles

…ntrib-vanilla into issue/469

… controls and strapline - Narrative strapline icon background color set to transparent to prevent icon background obscuring the btn focus outline

…ntrib-vanilla into issue/469

- border-radius added - default drawer item vars represent current drawer styles - margin gives option to display items in a 'button style' - when margin is set, apply border to whole of button (not just bottom)

- drawer item selected state expanded to support aria-current="true" (used by languagepicker which should inherit selected, not disabled styling.

properties.schema

schema/theme.schema.json

kirsty-hames

Thanks for updating schemas @guywillis. All looking good. I've just added a few text amends.

swashbuck · 2025-01-16T21:07:53Z

Hi, @guywillis . This is looking great. Thanks for the hard work. I have a few comments/questions below.

For disabled outline style buttons, I think I would prefer just a lighter border instead of a filled background. I do like the filled style on hover/focus, though.

In the Less filenames, I think the 'i' should be capitalized in 'UI' since it is an acronym. Ex: _drawerUIStyles.less not _drawerUiStyles.less. Microsoft says "When using acronyms, use Pascal case or camel case for acronyms more than two characters long. For example, use HtmlButton or htmlButton. However, you should capitalize acronyms that consist of only two characters, such as System.IO instead of System.Io." I'm sure you could find sources that disagree, of course.
core/questionButton.less - "button" should come first like core/buttonQuestion.less. This is similar to the notify/notifyPush and drawer/drawerItem Less files and would keep these button files together.
btn-color-outline(): Why use a box shadow and not an actual border? I get why we should not use outline since that should be reserved for focus states.
This may be out of scope, but I do not like that some variable names include "color" (ex. @menu-item-btn-color-hover) while others do not (ex. @progress-border). Without including "color" or something similar (ex. "bg-color"), it's unclear what the variable is for. For instance, @progress-border could define the border width or the entire border shorthand style.

Otherwise, great work!

less/_defaults/_spacing-config.less

guywillis · 2025-01-17T12:04:49Z

Hey @swashbuck, thank you for your comments and feedback.

For disabled outline style buttons, I think I would prefer just a lighter border instead of a filled background. I do like the filled style on hover/focus, though.

I agree, I think I would prefer this visual too. Our initial reasoning for keeping the filled disabled buttons for the outline style were two fold:

It provides a very clear visual indication that the button is disabled, especially in contrast to an active outline style button.
It provides simplicity in the Vanilla theme Less.

I would not be adverse to changing this but having experimented with mimicking this change in others button areas it looks weaker to me than the filled version.

Item

Filled	Outline

Item UI

Filled	Outline

Notify UI

Filled	Outline

In the Less filenames, I think the 'i' should be capitalized in 'UI' since it is an acronym. Ex: _drawerUIStyles.less not _drawerUiStyles.less. Microsoft says "When using acronyms, use Pascal case or camel case for acronyms more than two characters long. For example, use HtmlButton or htmlButton. However, you should capitalize acronyms that consist of only two characters, such as System.IO instead of System.Io." I'm sure you could find sources that disagree, of course.

I am not against such a change, it makes sense.

Reflecting upon the names of the files _itemStyles.less, _btnStyles.less etc, I think we can actually drop the Styles part and instead rename the folder from _buttonMixins to _buttonStyles

core/questionButton.less - "button" should come first like core/buttonQuestion.less. This is similar to the notify/notifyPush and drawer/drawerItem Less files and would keep these button files together.

This change makes a lot of sense, I am in favour.

btn-color-outline(): Why use a box shadow and not an actual border? I get why we should not use outline since that should be reserved for focus states.

Predominantly, I prefer box shadow over border as it's more flexible:

box shadow doesn't affect the elements width or height
it can be applied outside the dimensions of the element as well as the inside
it can support multiple values at once

This may be out of scope, but I do not like that some variable names include "color" (ex. @menu-item-btn-color-hover) while others do not (ex. @progress-border). Without including "color" or something similar (ex. "bg-color"), it's unclear what the variable is for. For instance, @progress-border could define the border width or the entire border shorthand style.

I am in agreement here. It should be consistent and flow together. I think it would be best if we addressed this as a separate issue.

swashbuck · 2025-01-17T15:23:59Z

Thanks, @guywillis . I agree with all your points. Fine with leaving the disabled outline styles as is. Should be simple enough to change in a custom theme.

kirsty-hames · 2025-01-20T17:37:07Z

Hi @guywillis / @swashbuck, I've had a read through all comments above and I'm in agreement. I just have one preference regarding the following,

Reflecting upon the names of the files _itemStyles.less, _btnStyles.less etc, I think we can actually drop the Styles part and instead rename the folder from _buttonMixins to _buttonStyles

I agree with dropping 'Styles' from the files but think it makes sense to keep the folder as _buttonMixins. As these files contain mixins only. 'Styles' is always a given with CSS anyway :)

kirsty-hames and others added 30 commits January 11, 2024 16:23

Content item focus and disabled state added

8df47a4

- focus state inherits from hover (as per existing component focus styles) - existing @visited vars are only used by components so grouped with content item

Content button focus, selected, locked and disabled state added

31c845f

- focus state inherits from hover (as per existing focus styles) - locked state inherits from disabled (as per existing locked styles)

Set up btn-ui-color variables

2d76066

Set up btn-color and btn-ui-color mixins

86ded18

Removed previous btn styling, added new mixin support

0902920

Added missing styling to plugins

fe1cd97

Narrative controls disabled fix

e2c978c

Added item-color mixins

c06e250

Added comments / questions

a3206dd

Added ui button controls mixin

b572119

- ui-btn-controls vars added - shared Notify and Drawer btn styles combined into single ui-btn--controls mixin. Replacing .notify-btn-icon mixin.

Added comments / questions

60cabbb

Menu item btn focus and locked state added

3c48fb9

Fix menu item btn locked state to inherit from disabled vars

98ae77b

notify ui btn mixins

18046ff

Add item ui mixin

9693002

- set menu item btn vars to inherit from item ui

Comments updated

e051567

Apply item ui mixin to plugin files

cb29593

- item ui locked state added - set .narrative__strapline-icon to inherit from narrative__strapline-btn to reduce additional state declaration styles

UI control mixins, nav btn padding

8fd5b33

Merge branch 'issue/469' of https://github.com/adaptlearning/adapt-co…

49ee8a7

…ntrib-vanilla into issue/469

Updated UI base inverted colour for aa contrast

179f634

Removed unused disabled state from boxMenuItem btn

6cffe36

Focus states added to Hotgraphic pin, Media transcript btn, Narrative…

49987b0

… controls and strapline - Narrative strapline icon background color set to transparent to prevent icon background obscuring the btn focus outline

Adjusted notify and drawer colour definitions

18c9618

Comment updates

4158bfb

Merge branch 'issue/469' of https://github.com/adaptlearning/adapt-co…

d21a1d8

…ntrib-vanilla into issue/469

Set @item-ui-color to inherit from @item-color by default

179bd74

Added drawer item color mixins

519156a

Add drawer item vars for margin, padding and border

b3551cf

- border-radius added - default drawer item vars represent current drawer styles - margin gives option to display items in a 'button style' - when margin is set, apply border to whole of button (not just bottom)

Drawer item focus state added

9782c7f

- drawer item selected state expanded to support aria-current="true" (used by languagepicker which should inherit selected, not disabled styling.

kirsty-hames assigned kirsty-hames and guywillis and unassigned guywillis Dec 18, 2024

kirsty-hames changed the title ~~Breaking: Adapt buttons overhaul (fixes #469)~~ New: Adapt buttons overhaul (fixes #469) Dec 18, 2024

guywillis added 4 commits January 6, 2025 11:38

Merge branch 'master' into issue/469

a3a6852

Notify / drawer outline updates

99688af

Removed unused variable

7c007f7

Schemas updated

38e6761