Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Define clearBoth class #66

Merged
merged 1 commit into from
Jun 30, 2021
Merged

Define clearBoth class #66

merged 1 commit into from
Jun 30, 2021

Conversation

scottcwilson
Copy link

@scottcwilson scottcwilson commented Jun 28, 2021

This class is used in a number of template files, both in bootstrap and template_default, so it needs to be defined.

Copy link
Owner

@lat9 lat9 left a comment

Choose a reason for hiding this comment

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

What is the underlying issue being corrected by this change?

@scottcwilson
Copy link
Author

scottcwilson commented Jun 29, 2021

  • clearBoth is referenced but not defined.
  • clearfix is referenced but not defined - but it's a duplicate.
    You can see the impact of not having a clearBoth definition by attempting to use it to clear floats- it won't clear them.

So if you do a clearboth and then add some text right underneath it (for example), the text will still be floated just like the item before it.

Prob easiest way to see is just put some content on an ezpage, float some things, do a clearboth and then add some content underneath that. You'll see the floats are not cleared.

@scottcwilson
Copy link
Author

Here's a way to dup the issue: If you modify bootstrap's version of tpl_product_info_display.php to add some text right after the clearBoth following Ask a Question, you'll see the text is still floated.

@lat9
Copy link
Owner

lat9 commented Jun 29, 2021

There needs to be a way to get that to render properly using clearfix. One of the tenets of the BootStrap template instituted by Ray Barbour is that the stylesheet.css is kept to a minimum and the Bootstrap classes are maintained.

@scottcwilson
Copy link
Author

Right now there are 43 references to clearBoth in bootstrap/templates, but this class is not defined so it won't work the way people expect. You could change all clearBoth classes to clearfix, but then you'd still be in trouble if template_default had a template which was not overridden that referred to clearBoth. I think my fix is the least intrusive but it's your call.

@lat9
Copy link
Owner

lat9 commented Jun 30, 2021

I can see the need for defining the clearBoth style, but I'm still not convinced that the associated template changes are required. I'm open to the change if you can show me something that's not working.

@scottcwilson
Copy link
Author

Nah, I'll back that out. So now the PR has just the CSS change.

@scottcwilson scottcwilson changed the title Define and use clearBoth (not clearfix) class Define clearBoth class Jun 30, 2021
@lat9 lat9 merged commit c2ed7f9 into lat9:v300 Jun 30, 2021
@scottcwilson scottcwilson deleted the clearboth branch June 30, 2021 20:33
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.

2 participants