Add modal to new home page #242
Conversation
|
@garg3133 made a pr for this already but there were too many conflicts so i decided to make a new one. |
|
@cryptofox17 Sorry for telling you this late, but can you use the Bootstrap modals for this issue? We are using bootstrap on our new home page, which we were not using in the older home page, that's why we needed to implement everything ourselves. But as we are using Bootstrap on this page, implementing the Donate Us modal using bootstrap would be a lot cleaner and easier to read. |
|
Other than that, the design looks good. Just a little space is required above that OR line and you can avoid using that circle around the close button, which looks a bit odd. |
|
@garg3133 I have removed most of the custom css and added required bootstrap classes instead |
|
@cryptofox17 I not only meant using bootstrap classes for stuff line margins, centre-aligning, etc., but I also meant using Bootstrap for modals itself. See https://www.w3schools.com/bootstrap4/bootstrap_modal.asp |
|
@garg3133 done
|
|
@cryptofox17 I am currently busy with my college project and assignment submissions followed by my end-sem exams, so I might not be available for a few days to review your PR. Sorry for the delay. |
|
No problem, good luck with the exams |
There was a problem hiding this comment.
Hey @cryptofox17, sorry for such a late review. I have reviewed your PR and it was almost perfect! I have posted a few suggestions kindly check.
Also @cryptofox17 there is a major bug in your PR, whenever I open the modal and close it, it literally adds the padding in the body, I am attaching the screenshot for reference
Can you see that right padding? (Image is after 5-6 attempts of opening and closing the Modal)
Actually, it's the issue of bootstrap itself, and they have one PR open for it, but don't know when they will merge it, till then you can fix it manually.
templates/new_home/index.html
Outdated
| <h4>Account No :</h4> | ||
| <p>xxxxxxxxxxx</p> | ||
| <h4>IFSC Code :</h4> | ||
| <p>XXXXxxxxxxx</p> |
There was a problem hiding this comment.
Can you please put starting capital X as regular x, because the details are not going to change soon and it's not looking good from a design perspective.
templates/new_home/index.html
Outdated
| kids. Contact: | ||
| </p> | ||
| <div class="row mb-2"> | ||
| <div class="col-lg-6 text-center my-2"> |
There was a problem hiding this comment.
Can you use col-md-6 instead of col-lg-6, it's better in that way.
| <span aria-hidden="true">×</span> | ||
| </button> | ||
| </div> | ||
| <div class="modal-body container"> |
There was a problem hiding this comment.
Can you change the background color from white to #FEF8F5 because that's the color I have used in the Figma File?
| <div class="modal fade" id="don-modal" tabindex="-1" role="dialog" aria-labelledby="don_modal" aria-hidden="true"> | ||
| <div class="modal-dialog" role="document"> | ||
| <div class="modal-content"> | ||
| <div class="modal-header text-center"> |
There was a problem hiding this comment.
Can you add Shadow here? Shadow in header, title, and the cross button, can you?
|
Also one thing I forgot to add, the color of the |
|
@tushhr done, let me know if its fine now
|
|
Awesome @cryptofox17! Sorry for the inconvenience and thanks for your contribution🎉 |
Related Issue
Fixes: #207
Prosposed Changes
Screenshots