Skip to content

Conversation

MGatner
Copy link
Member

@MGatner MGatner commented Mar 12, 2019

Description
The user guide claims "The Pager class will render a series of links that are compatible with the Bootstrap CSS framework by default" but by default the resulting HTML is missing the necessary classes to be rendered as a Bootstrap pagination.

Checklist:

  • Securely signed commits
  • Component(s) with PHPdocs
  • Unit testing, with >80% coverage
  • User guide updated
  • Conforms to style guide

@jim-parry
Copy link
Contributor

jim-parry commented Mar 12, 2019

The user guide is poorly worded. We don't want to add CSS framework attributes.
Way back when, Bootstrap prided itself on styling most HTML elements without additional attributes, and my guess is that is where the original user guide wording came from. I remember using unordered lists for navbars, simply wrapping them in a suitable bootstrap-styled component. That could also be the source of the original "claim".
Better is to remove any mention of Bootstrap compatibility, which is misleading.

@jim-parry jim-parry closed this Mar 12, 2019
@jim-parry
Copy link
Contributor

This looks like a can of worms! There are several nav styles or pagination support built into Bootstrap, slightly different per bootstrap version - default, links, pills, pager vs plain nav.

The default_simple and default_head pagination templates seem CSS-framework-free, but not so default_full. The only other place in the user guide which mentions Bootstrap is the validation library. Looking at its views, it adds a couple of CSS attributes to error message divs.

@lonnieezell
Copy link
Member

Yes, that was originally for Bootstrap 3. Looks like 4 took a step backward in that respect. Too bad.

@jim-parry
Copy link
Contributor

So, should the bootstrap styling be removed?
if we leave it in there, will the community expect it everywhere?
... not to mention "what about bootstrap X or each's favorite CSS framework" :(

@MGatner
Copy link
Member Author

MGatner commented Mar 13, 2019

They need some classes so users can apply CSS to the out-of-the-box HTML. That said, the “pagination” class on the wrapping NAV ought to be minimum necessary. It is indeed unfortunate that this is no longer enough for Bootstrap, but applying their specific classes to the items definitely seems like a decision to “go with Bootstrap”.
The alternative is also unfortunate though - requiring a user to create a custom pagination view just because they want classes added to otherwise acceptable HTML. Since (presumably) one would want pagination classes consistent across an app, what about adding some class options to Pager.php, especially since it already exists? That was the first place I went and looked when I realized the items were missing classes.

@lonnieezell
Copy link
Member

The easy solution for people asking for support of X is to simply say, we only support Y because it seems to be the most widely used and makes a good default.

However, in this case I say we leave it like it is so that it can be styled without modification, and remove the comment from the docs.

@MGatner
Copy link
Member Author

MGatner commented Mar 13, 2019

I will send a PR for the user docs.

MGatner added a commit to MGatner/CodeIgniter4 that referenced this pull request Mar 13, 2019
@MGatner
Copy link
Member Author

MGatner commented Mar 13, 2019

PR sent. FYI the only remaining reference to Bootstrap styling in the User Docs is in the Validation Library (https://codeigniter4.github.io/CodeIgniter4/libraries/validation.html), the views called by $validation->listErrors() and $validation->showError()"display in a manner compatible with the Bootstrap CSS framework".

lonnieezell added a commit that referenced this pull request Mar 14, 2019
Remove bootstrap styles from validation views. #1816
@lonnieezell
Copy link
Member

I've removed the Bootstrap reference and changed the style to be a bit more generic.

@MGatner MGatner deleted the pager-bootstrap-classes branch March 20, 2019 14:48
NEKOGET pushed a commit to NEKOGET/ci4_user_guide_src that referenced this pull request Jul 4, 2020
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.

3 participants