-
-
Notifications
You must be signed in to change notification settings - Fork 279
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
Fixed responsiveness of buttons in create-collection page #527
Conversation
@MonkeyDo Please kindly review 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, good find!
I have a suggestion for a different approach:
First of all, let's fix the issue with the overlapping buttons in a way that will fix it across the website. I've identified the issue is coming from the bootstrap theme, but can (and has) been mitigated; see this bit of CSS used previously to sort the issue of horizontal overlap:
https://github.com/bookbrainz/bookbrainz-site/blob/master/src/client/stylesheets/style.less#L83-L87
I propose to change that simply to margin: 0;
to fix the overlapping issue vertically as well.
With that out of the way, I think there is a simple way to display our buttons properly using existing css.
We can use bootstrap's responsive grid system by modifying the structure just a bit:
<div class="row">
<div class="col-sm-6">
<button type="button" class="btn btn-block btn-primary">Add another collaborator</button>
</div>
<div class="col-sm-6">
<button type="submit" class="btn btn-block btn-success">Create collection</button>
</div>
</div>
- We change the
text-center
class of the parent element torow
- We create two
<div class="col-sm-6">
elements around the existing buttons - Profit.
I think it looks quite good like that, with full width buttons in mobile sizes, and better looking as well in desktop sizes I find:
How does that sound?
@MonkeyDo https://getbootstrap.com/docs/4.0/layout/grid/#no-gutters also , i think giving space between two buttons will look more nicer so i think we can use above "no-gutter" method and then add the space manually in this section . what do you think? |
I'm not sure I understand your suggestion. Which .row element do you want to modify? We're using an older version of Bootstrap that doesn't have the |
yes i agree on this . also
looks great to me , |
@MonkeyDo please review the changes ! |
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.
It's looking quite good !
A suggestion below to simplify the fix.
Also, currently our continuous integration (CI) tests are failing on simple code style issues, which you can easily fix by running this command: npm run lint -- --fix
before committing the changes to the userCollection.js
file.
@@ -262,7 +262,8 @@ class UserCollectionForm extends React.Component { | |||
<div className={errorAlertClass}> | |||
<Alert bsStyle="danger">Error: {errorText}</Alert> | |||
</div> | |||
<div className="text-center"> | |||
<div className="row"> | |||
<div className = "col-sm-6"> |
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.
You can add a simple Bootstrap class to add a space on top of both of these col-sm-6
elements, which will give a bit of space between the buttons when they are on top of each other:
<div className = "col-sm-6"> | |
<div className = "col-sm-6 margin-top-d5"> |
With that simply change on both elements, you can remove the changes to the style.less
file.
src/client/stylesheets/style.less
Outdated
@@ -549,3 +550,10 @@ hr.wide { | |||
margin-right: 0.5em !important;; | |||
vertical-align: text-top; | |||
} | |||
|
|||
@media (max-width: 498px){ | |||
.buttons-container > button:nth-child(2) { |
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.
This buttons-container
class is not used anywhere, so this code block is unused, and can be removed.
As mentioned in the comment above, we can add a bit of space between the buttons using existing bootstrap classes instead.
@MonkeyDo Please review the changes! |
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.
Just two more details and I think this will be perfect :)
> | ||
<FontAwesomeIcon icon="save"/> {submitLabel} | ||
</Button> | ||
<div className="row"> |
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.
<div className="row"> | |
<div className="row margin-bottom-2"> |
This makes sure there's a bit of space between the buttons and the footer.
{ | ||
this.props.collection.id ? | ||
<Button |
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.
I just noticed the third button ("Delete"), which is only visible when you edit one of your own collections.
It should receive the same changes as the other buttons, meaning it should also be wrapped in a <div>
in the same way and the button should have the block
attribute.
Here's the full block of code:
{
this.props.collection.id ?
<div className="col-sm-6 margin-top-d5">
<Button
block
bsStyle="danger"
type="button"
onClick={this.handleShowModal}
>
<FontAwesomeIcon icon="trash-alt"/> Delete collection
</Button>
</div> : null
}
With three buttons in mind, it's worth you having another pass to see if it looks good with the three buttons.
@MonkeyDo Sorry for the late commit, I was occupied with some work, I have made the requested changes, feel free to let me know if you need any further changes. |
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.
Thank you for your PR, this now looks a whole lot better !
🚀
Congratulations on your first merged pull request for BookBrainz! 🎉 |
Problem
https://tickets.metabrainz.org/browse/BB-558
![WhatsApp Image 2020-10-27 at 2 27 13 PM](https://user-images.githubusercontent.com/64382399/97345330-d21e9700-1881-11eb-9e10-b3992df1aaf2.jpeg)
Solution
Applied top margin for mobile screens using media query.
Areas of Impact
src/client/components/forms/userCollection.js
src/client/stylesheets/style.less