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

Change in modal UI #208

Merged
merged 7 commits into from
Apr 17, 2021
Merged

Change in modal UI #208

merged 7 commits into from
Apr 17, 2021

Conversation

cryptofox17
Copy link
Contributor

@cryptofox17 cryptofox17 commented Mar 30, 2021

Related Issue

Fixes: #207

Prosposed Changes

  • Changes the color scheme of the modal
  • Changes the text alignment

Additional Info

  • Ensured that it is responsive

Screenshots

Original Updated
old new
old new

@garg3133
Copy link
Owner

@cryptofox17 The design seems fine, except for the orange background.

Also, it seems like you didn't follow the discussion in #23. I proposed some changes in that issue.

@cryptofox17
Copy link
Contributor Author

@garg3133 ok I'll make the changes

@cryptofox17
Copy link
Contributor Author

@garg3133 let me know if the background and alignment is ok now
Screenshot (17)
Screenshot (18)

@garg3133
Copy link
Owner

garg3133 commented Apr 1, 2021

Can you centre-align everything and then show it to me?

Also, don't put the heading "Contact" in the second section. Instead, you can put a "---OR---" or something like that in between and then start the new section as "You may also donate books, clothes, stationery items or anything else which might be useful to the kids. Contact:
Name 1 / Phone no. / email
Name 2 / Phone no. / email
for making your donations.

@cryptofox17
Copy link
Contributor Author

@garg3133 I have modified the contact section. I tried aligning everything to the center but it looked a bit off to me so I haven't committed the change. But here are the screenshots :

  • current
    Screenshot (121)

  • center aligned
    Screenshot (118)

@garg3133
Copy link
Owner

garg3133 commented Apr 2, 2021

I think this looks better:

image

I've used CSS-Grids in place of Flexbox.

HTML

<div class="flex-item d-flex">
    <h4>Account Name :</h4>
    <p>JAGRATI</p>
    <h4>Account No :</h4>
    <p>xxxxxxxxxxx</p>
    <h4>IFSC Code :</h4>
    <p>XXXXxxxxxxx</p>
</div>

CSS

.flex-item {
	display: grid;
	grid-template-columns: 1fr 1fr;
        grid-auto-rows: 40px;
	align-items: center;
	/* height: 36px;
	margin: auto; */
}

don-box2 h4 {
	font-size: 14px;
	/* margin: 0 12px 0 0 !important; */
	text-transform: uppercase;
	color: #ff8c00;
	justify-self: right;
}

.don-box2 p{
	margin-left: 15px;
	color: #4e4e4e;
	font-weight: 400;
	justify-self: left;
}

.don-box2 > h2 {
	margin-bottom: 10px;
	font-weight: 700;
	color: #000000;
	letter-spacing: 0.5;
}

Do change the class names appropriately.

@tushhr Thoughts?

@cryptofox17
Copy link
Contributor Author

@garg3133 made the changes.
Screenshot (122)

@tushhr
Copy link
Collaborator

tushhr commented Apr 2, 2021

Hey @cryptofox17!
I think both the donation modal doesn't convey the message in an efficient form. I'd say you should wait for some time until we settle down for one good design.

I will surely ping you back when our design will be ready.

@cryptofox17
Copy link
Contributor Author

@tushhr ok then!

@tushhr
Copy link
Collaborator

tushhr commented Apr 16, 2021

Hey @cryptofox17! Sorry for such a late reply. We have two designs for the donation modal one for the new home page and the second for the old one.

Link to the Figma file.

Just an edit, for the donation modal of the old home page, keep the header just like the one you have created in the beginning.

@cryptofox17
Copy link
Contributor Author

@tushhr which header?
Also, I have to just make for the old homepage right, which is the second one?

@tushhr
Copy link
Collaborator

tushhr commented Apr 16, 2021

This one
image

Yeah, but we don't have any Donation modal for the new home page, available at /new could you create one for that too?

@cryptofox17
Copy link
Contributor Author

@tushhr yeah I can. And ok

@garg3133
Copy link
Owner

@cryptofox17 You can create two separate PRs for the old and new home page.

@cryptofox17
Copy link
Contributor Author

done @garg3133 @tushhr
Screenshot (142)
Screenshot (141)

Copy link
Owner

@garg3133 garg3133 left a comment

Choose a reason for hiding this comment

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

This looks great @cryptofox17! Just a few things:

  • The scrollbar looks odd on small screens. Can you hide the scrollbar on small screens, using media queries? This might be helpful: https://www.w3schools.com/howto/howto_css_hide_scrollbars.asp
  • Add the following to the last line: useful to the kids. Contact:
  • On clicking the Donate link in navbar, the page scrolls to the top. This is happening because of # in href attribute of the link. Replace # with javascript:void(0) there.

static/home/css/style.css Show resolved Hide resolved
static/home/css/style.css Show resolved Hide resolved
static/home/css/style.css Show resolved Hide resolved
margin-bottom: 12px;
margin-top: 12px;
margin-right: 64px;
}
Copy link
Owner

Choose a reason for hiding this comment

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

As the properties for both .details-1 and details-2 are same, it can just be one single class .contact-details.

Also, prefix the class with don-box3 so that it does not conflict with other elements with same class name (if we use this class name at any other place on the page).

static/home/css/style.css Outdated Show resolved Hide resolved
templates/home/index.html Outdated Show resolved Hide resolved
templates/home/index.html Outdated Show resolved Hide resolved
@cryptofox17
Copy link
Contributor Author

@garg3133 done

@cryptofox17
Copy link
Contributor Author

Screenshot (146)
Screenshot (145)

margin-right: 64px;
}

.don-box3-p {
.or {
Copy link
Owner

Choose a reason for hiding this comment

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

You should write this class as .don-box3 .or, as .or class can appear at other places too, so you must be specific that these properties should only apply to the .or class inside .don-box3 class and not anywhere else.

text-align: center;
max-width: 150px;
margin-bottom: 12px;
margin-top: 12px;

}
Copy link
Owner

Choose a reason for hiding this comment

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

I think you didn't understand well what I suggested earlier.

  • As both the classes have some common properties, you can put them in a single class name .contact-details (put this class in both the elements).
  • Now for the different margins of two divs, put a class name .contact-details-left in the left div and class name .contact-details-right in the right div and apply corresponding margins to both the classes.
  • Add .don-box3 before both the classes in CSS, so that these properties only apply to .contact-details, .contact-details-left and contact-details-right classes inside the .don-box3 and not anywhere else if we use the same class name somewhere else. Ex. .don-box3 .contact-details.

@cryptofox17
Copy link
Contributor Author

@garg3133 done.

@garg3133 garg3133 merged commit a1c2902 into garg3133:master Apr 17, 2021
@garg3133
Copy link
Owner

Looks good now, for the most part. Although I think you still didn't understand what I was trying to say, but no worries, I've made those changes myself. You can look at them here: 570b423

Merged your PR into master 🎉

Thanks, @cryptofox17 for your contribution 🚀

@cryptofox17 cryptofox17 deleted the modal_UI_change branch April 18, 2021 11:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improving the UI donate modal
3 participants