Skip to content

Conversation

@samueljlieber
Copy link
Contributor

@samueljlieber samueljlieber commented Jan 23, 2024

This PR fixes "simple" bullet list spacing within a non-simple list.
This issue was discovered in #7301 (comment)

Screenshot 2024-02-05 at 12 08 55 PM

This PR is addressing is the lack of bottom margin on the last <p> tag in a .simple list.

@samueljlieber samueljlieber self-assigned this Jan 23, 2024
@robodoo
Copy link
Collaborator

robodoo commented Jan 23, 2024

@C3POdoo C3POdoo requested review from a team January 23, 2024 16:50
@samueljlieber samueljlieber force-pushed the 15.0-scss-sub-bullet-fix-sali branch 2 times, most recently from d195a1a to 466653e Compare February 7, 2024 17:41
Copy link

@Brieuc-brd Brieuc-brd left a comment

Choose a reason for hiding this comment

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

Hey 👋

So the issue this PR is addressing is the lack of bottom margin on the last

tag in a .simple list.

There is indeed a lack of bottom margin for .simple lists, but to solve this, you don't have to apply the margins on the last p. You can target a simpler selector that will be much easier to maintain 😉 (comments below).

@samueljlieber samueljlieber force-pushed the 15.0-scss-sub-bullet-fix-sali branch from 466653e to ecf4ea0 Compare February 9, 2024 14:23
@samueljlieber
Copy link
Contributor Author

Hi @Brieuc-brd, thanks again for your insightful review! I agree I do not need to use such a complex selector, my reasoning for doing so was to replace this ⬇️ existing rule to have a single (although more complex) rule:

ol.simple p,
ul.simple p {
    margin-bottom: 0;
}

However, if I leave this ⬆️ existing rule, and add the simple selector you suggested, the styles works as intended!

Please take a look at the Files Changed tab on this PR to see the placement of the rule. I also removed two rules that were removing the margin-top on paragraphs, as you pointed out this is already taken care of by _reboot.scss.

Thank you for your insight here! 🙂

Comment on lines 601 to 610
ol.simple p,
ul.simple p {
margin-bottom: 0;
}

li > .simple {
margin-bottom: 1em;
}

Choose a reason for hiding this comment

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

We could go further, I'd use .simple as the main selector and use the power of SCSS 🙂

.simple {
    p {
        margin-bottom: 0;
    }
    
    li > & {
        margin-bottom: 1em;
    }
}

But, again, is the simple class used on other elements than ul & ol ? @AntoineVDV maybe you know it? 🙂

Copy link
Collaborator

Choose a reason for hiding this comment

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

No idea^^

Copy link
Contributor Author

@samueljlieber samueljlieber Feb 12, 2024

Choose a reason for hiding this comment

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

@Brieuc-brd .simple is also used on dl elements. This SCSS works well for the dl element too.

I searched through the documentation and did not find any other instances of where the class .simple is used on other elements. This docutils Examples of Syntax Constructs document also only shows the class being used on only the ul, ol, and dl elements.

The dl, dt, and dd elements are heavily used on the applications/finance/accounting/get_started/cheat_sheet.rst doc for example.

I will push up a commit to update the CSS to SCSS since it is a more elegant solution 👍

@samueljlieber samueljlieber force-pushed the 15.0-scss-sub-bullet-fix-sali branch from ecf4ea0 to 287d5fa Compare February 12, 2024 15:19
Copy link

@Brieuc-brd Brieuc-brd left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@AntoineVDV AntoineVDV removed the request for review from a team February 13, 2024 08:54
Copy link
Collaborator

@AntoineVDV AntoineVDV left a comment

Choose a reason for hiding this comment

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

Thanks @Brieuc-brd for reviewing :)

This also adds a margin to nested lists that are the last list item of the parent list, but I think that it's okay.

Could you just change the commit message to something more Odooesque? For example [FIX] odoo_theme: add bottom margin to inner lists

@robodoo delegate+

@samueljlieber samueljlieber force-pushed the 15.0-scss-sub-bullet-fix-sali branch from 287d5fa to 38dcb52 Compare February 13, 2024 14:09
@samueljlieber samueljlieber changed the title [FIX][CSS]: ul ol sub-bullet margins [FIX] odoo_theme: add bottom margin to inner lists Feb 13, 2024
@samueljlieber
Copy link
Contributor Author

Updated commit message in 38dcb52.

Thank you both for the insight and reviews 🙏

@robodoo r+

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.

4 participants