-
Notifications
You must be signed in to change notification settings - Fork 433
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
Utility gap-* classes for flexbox #2364
Conversation
5ab10d1
to
b5ed6f2
Compare
@davide-negretti and @atarix83 : I'm just now getting back to this PR. However, I'm a little concerned about adding these custom Currently, we are still on Bootstrap 4 ( So, I'm not sure myself whether to move this forward, as I feel it starts to pull Bootstrap 5 concepts back into our basic Bootstrap 4 styling. I'd welcome your opinions though, and we could bring this to a future DevMtg discussion if we feel this direction is worthwhile. |
@tdonohue I didn't know that these classes were already part of Bootstrap 5. I introduced them for flex layout ( I wouldn't call this an anticipation of Boostrap 5 features because my classes are being used for a different purpose (the grid layout feature is not present on Boostrap 4.6 and I'm not introducing it). I think that it is safe to introduce them alongside Boostrap 4.6, as I made sure that:
|
@davide-negretti : I think the question then becomes whether we want to be adding custom utilities on top of Bootstrap (in general). If so, I'm not sure whether we should embed them all into These I have to admit, I'm very torn on this approach of adding our own custom utility classes. I completely understand the usefulness of sharing CSS utilities. But, at the same time, the more we diverge from "basic Bootstrap CSS", the more we will have to create our own DSpace CSS documentation separate from Bootstrap. (And I'd rather we not go too far down that path, as we want developers familiar with Bootstrap to be able to easily theme DSpace.) So, I still feel this PR needs discussion as it's a "new direction" for DSpace. Do we want to have our own set of "utility" CSS classes, or do we only use those defined in out-of-the-box Bootstrap? I'd appreciate feedback from @atarix83 and @artlowel as two of our key Angular experts (and designers of much of the Angular UI). |
Hi @davide-negretti, |
b5ed6f2
to
e0d2ab7
Compare
@tdonohue while working on another issue I noticed that similar classes have been added for this PR: #2527 This is one of the many cases where these |
e0d2ab7
to
930a381
Compare
@davide-negretti : Good point that it appears #2527 could have used these classes. Maybe we should update the (now merged) code from #2527 to utilize these new In any case, it does appear these could have general use as you had recommended. I'm fine with this moving forward, just not sure if we want to give an example or two of usage in our codebase? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 Thanks @davide-negretti . After looking at this more and noticing that others are creating similar utility classes both in #2527 as well as #2594, I think I'm OK with moving this forward. It's not used yet in our codebase, but it's obvious that we've found usefulness out of similar utility CSS classes, so it seems reasonable to add these.
I'll leave this open a few more workdays though to allow others final comments. Then I'll merge into 7.6.1 (and 8.0)
Hi @tdonohue, I just added a couple of example to this PR |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @davide-negretti!
I agree, these are useful. We just have to keep in mind to remove them when we upgrade to bootstrap 5
Thanks again @davide-negretti ! This looks good & I've verified the minor examples in the |
Successfully created backport PR for |
Description
This PR adds some Bootstrap-style utility classes to handle horizontal and vertical gaps between flexbox elements.
A
gap-*
class adds a space between elements of the same size ofm-*
andp-*
Bootstrap classes, whilegapx-*
andgapy-*
classes allow to set horizontal and vertical gaps.These classes will allow to handle spacing between many elents of the DSpace UI in a simpler way (currently the spacing is managed by manually adding margins to each element).
For example, in
dso-edit-menu.component.html
:Instructions for Reviewers
Create a flex container and add gap-* classes to test them, e.g.
Checklist
yarn lint
yarn check-circ-deps
)package.json
), I've made sure their licenses align with the DSpace BSD License based on the Licensing of Contributions documentation.