item-margin: accordion, matching, mcq, textInput

adaptlearning / adapt_framework

A toolkit for creating responsive, accessible, multilanguage HTML5 e-learning courses.

https://www.adaptlearning.org/

GNU General Public License v3.0

570 stars 243 forks source link

item-margin: accordion, matching, mcq, textInput #1500

Closed chucklorenz closed 4 years ago

chucklorenz commented 7 years ago

Several components stack their items: Accordion, Matching, MCQ, and Text Input. The Less that describes the spacing between items appears to be inconsistent: Accordion: margin-bottom: @item-margin-bottom; Matching: padding-bottom: @item-padding-bottom; MCQ: margin-bottom:2px; Text Input: margin-bottom: 2px;

It seems that MCQ and Text Input ought to use variables. And it seems that all four should be using the same variable. As this space is meant to separate items and is not meant to be a part of the item itself, “margin” would be more appropriate than “padding”: @item-margin-bottom.

Other changes:

Matching's padding-bottom should probably change to margin-bottom.
MCQ has itsmargin-bottom assigned in .mcq-item label. It ought to move to .mcq-item for consistency. (The effect is the same.)

Agreed?

prajaktakitukale commented 7 years ago

Hi @chucklorenz I was trying to resolve this bug and found that margin-bottom is assigned in .mcq-item label in vanilla theme as well so do we have to move margin-bottom from theme as well or make the changes in mcq component only?

chucklorenz commented 7 years ago

Hi @prajaktakitukale Thanks for jumping on this so quickly. Yes, I think it ought to change in Vanilla, too.

Just yesterday I was in a conversation that included @guywillis and @kirsty-hames --two persons that have played a role in Adapt theming and FED issues. The conversation raised issues, but did not get to the point of resolving them. Some issues:

Does this constitute a breaking change for these plug-ins?
Similar inconsistencies have been noticed. Should this issue (#1500) be rolled up into a wider effort to achieve consistent use of Less variables across all plug-ins?
Would consistency be better enforced by moving some component styling out of plug-in code and into core code?
Will piecemeal attempts at resolving these issues be annoying to users, especially to users of the authoring tool? Is it better to hold onto issues such as this one and resolve it in an effort that can be timed with version 3 of the framework?

I am undecided myself. Part of me wants rapid open source development that allows a plug-in to be its own entity with its own release schedule. Part of me respects that these plug-ins (and Vanilla) are core; that users tend to assume that not much is changing within their code; and that work done now (such as the coding you just did) might be reworked in the near future as part of v3.

What do you think?

prajaktakitukale commented 7 years ago

Hi @chucklorenz Thank you for the quick reply. Sorry for getting back to late, got stuck in some other work. We also found few more inconsistencies in other plugins like adapt-contrib-gmcq. I agree if we remove these overriding changes from theme and maintain the consistencies into core plugin. we also prefer to have these changes in v3.

moloko commented 5 years ago

still relevant?

guywillis commented 4 years ago

I believe, with the introduction of v5, all of the plugin spacing definitions use the @item-margin variable within the Vanilla theme.