bmbrands / theme_bootstrap

A Moodle theme based on the Bootstrap CSS framework
113 stars 112 forks source link

Numbered and bullet point lists not showing. #441

Closed gjb2048 closed 8 years ago

gjb2048 commented 8 years ago

Hi Bas,

Shoehorn issue https://github.com/gjb2048/moodle-theme_shoehorn/issues/25 also applies to Bootstrap in the M3.1 Stable branch. All down to missing margins. When I sort there I'll patch here too.

G

dasistwas commented 8 years ago

Hi, I think this issue needs still some attention. I kept on thinking about what is the difference between user generated lists and Moodle list elements for structuring the layout. And there is one differencein the user lists: The absence of attributes, especially the "class" attribute. So the challenge was only to find the appropriate selector. The :not selector combined with the attribute selector then provided an excellent solution. If you include the following lines in type.less (or whereever convenient), it solves all user created list problems. No matter where they appear (block, editor, course page, activity,...)

ul:not([class]) {
  list-style-type:disc;
}
ol:not([class]) {
  list-style-type:decimal;
}
ul:not([class]), ol:not([class]) {
    padding: 0;
    list-style-position:inside;
}   
ul:not([class]) ul, ol:not([class]) ul {
  list-style-type:circle;
  list-style-position:inside;
}
ol:not([class]) ol, ul:not([class]) ol {
  list-style-type:lower-latin;
  list-style-position:inside;
}

.dir-ltr {
    ul:not([class]), ol:not([class]) {
        margin: 0 0 10px 25px;
        li:not([class]) {
            padding-left: 1em;
            text-indent: -1em;
        }
    }
    ul:not([class]) ul, ol:not([class]) ul {
        margin-left:15px;
    }
    ol:not([class]) ol, ul:not([class]) ol {
        margin-left:15px;
    }
}
.dir-rtl {
    ul:not([class]), ol:not([class])  {
        margin: 0 25px 10px 0;
        li:not([class]) {
            padding-right: 1em;
            text-indent: -1em;
        }
    }
    ul:not([class]) ul, ol:not([class]) ul {
        margin-right: 15px;
    }
    ol:not([class]) ol, ul:not([class]) ol {
        margin-right:15px;
    }
}
mjsamberg commented 8 years ago

@dasistwas This solution worked perfectly for me. It might be a good addition to the base spreadsheet. Thank you!

gjb2048 commented 8 years ago

Interesting @dasistwas - but seems to be overkill! If you have to write such tortuous LESS as that then that smacks of something seriously wrong and in dire need of refactoring.

dasistwas commented 8 years ago

@gjb2048 The problem is, when you define a default ul, ol and li styling, then this is not fully overridden by the list elements used in the layout. This leads to random bullet points and weird margins in the whole layout. I think that will be far more work to resolve (maybe with mixins) compared to this little LESS. Basically it just tells: Do not look at lists which have classes. Maybe the first part can be omitted, which defines list-style-types. I only copied this from your solution in https://github.com/gjb2048/moodle-theme_shoehorn/commit/3f176b6ff26bb474e5f99e416da9f16c0fb83a37 Also text-indent/padding is problematic in some cases. There are other solutions for that like http://stackoverflow.com/questions/10428720/how-to-keep-indent-for-second-line-in-ordered-lists-via-css but maybe that leads to other problems in the layout....

This is for sure not the best solution, but I think it is efficient and fast and better than staying with the current problem. The problem with targeting specific areas in moodle like you did with .course-content li will for example not work for text-blocks, where you have user generated html in the block (that is not part of .course-content). And there are a bunch of other problems with this approach (I tested this with bootstrap theme, but maybe shoehorn is different).

mjsamberg commented 8 years ago

Exactly. I was trying to address this with targeting .course-content li before I saw your patch, but if you're using Bootstrap elements (like pills or tabs), there are some elements that then have to be reverted explicitly, and you hope you're able to find all of them....

gjb2048 commented 8 years ago

Ah ha, but https://github.com/gjb2048/moodle-theme_shoehorn/commit/3f176b6ff26bb474e5f99e416da9f16c0fb83a37 is a direct copy from the Moodle styles: https://github.com/moodle/moodle/blob/MOODLE_31_STABLE/theme/bootstrapbase/less/moodle/course.less#L371-L396 to solve the specific issue. Thus more of a maintenance solution whereby many more eyes ponder the problems and fix them, then just a matter of keeping up to date with the changes. Perhaps you could suggest your solution as an enhancement? As surely the Moodle CSS needs refactoring and reducing the amount to be more component orientated and not so 'lets style this grain of sand'.

dasistwas commented 8 years ago

As long as Moodle does not have a clear styling strategy or concept, this will stay messy forever. I am convinced, that Moodle CSS can be reduced by 50% to 80%, if they put a little effort in developing a sustainable layout, design and UI strategy, that focuses also on code quality, simplification and unification. What users complain most about Moodle is the UI. But I do not think, I can change the sand grain strategy of Moodle ;-).

gjb2048 commented 8 years ago

I totally agree David. I've been trying for years. I wonder if there is another way to convince them?

stuartlamour commented 8 years ago

from the snap css (which seems to work for most cases)

// shame - moodle uses ul and li for sections. We have to overspecify and reset the basic dom element styles to account for this .section { ul, ol { margin: 1.6em 4%;

    ul,
    ol {
        margin: 0 4%;
    }
}

.activityinstance .contentafterlink,
.activityinstance .contentwithoutlink {
    ul,
    ol {
        margin: 1.6em 4%;

        ul,
        ol {
            margin: 0 4%;
        }
    }

    .contentafterlink p {
        margin-bottom: 1.6em;
    }
}

}