search

overhangio / tutor-indigo

An elegant, customizable theme for Open edX
GNU Affero General Public License v3.0
74 stars 276 forks source link

There is already an <h1> on the pages #33

Closed misilot closed 1 year ago

misilot commented 2 years ago

There is already an <h1> in the template being rendered. There should only be one <h1> on a webpage with <h1 class="header-logo">

regisb commented 2 years ago

Can you share before/after screenshots of the affected pages?

misilot commented 2 years ago

Sure, I can try and get those! It looks like some css changes will probably required to make it look the same.

The biggest benefit to only having the single <h1> tag is for screen readers.

regisb commented 2 years ago

Sure, I understand the benefit. And it's probably ok if there are some visual changes. I'd just like to see a before/after to make sure that the changes are not too drastic.

regisb commented 2 years ago

@misilot did you see my comment above?

I'd just like to see a before/after to make sure that the changes are not too drastic.

regisb commented 1 year ago

Here are the visual differences:

About page with h1 (old): about-h1 About page with h2 (new): about-h2

Contact page with h1 (old): contact-h2 Contact page with h2 (new): contact-h1

As much as I support the change for the sake of document structure, the visual changes are simply not acceptable as they stand.

Actually, the h1 tags are already present in the default edx-platform theme: https://github.com/openedx/edx-platform/blob/master/lms/templates/index_overlay.html https://github.com/openedx/edx-platform/blob/master/lms/templates/static_templates/contact.html

Thus, the changes should probably be made upstream first, before being applied here.

I'm closing this PR for now. But I'll be happy to reopen if you decide either to make the changes upstream or to introduce CSS changes here.