search

NEAR-Edu / near-certification-tools

2 stars 2 forks source link

Feature/update-footer-order-align #9

Closed ozanisgor closed 2 years ago

ozanisgor commented 2 years ago

@ryancwalsh according to https://github.com/NEAR-Edu/near-certification-tools/pull/7#issuecomment-1041509218 I did below changes.

  1. Update footer content order to Logo > Icons > Jobs
  2. Align footer left (In this point I am not sure did I understand correctly)
    • For this change I remove the space at left and right for the entire footer but I might be misunderstood so because of that I did an alternative version which align the entire footer to left and creates a space at right.

17 02 22_layout_before

17 02 22_layout_after

2nd Version

17 02 22_layout_after_v2

ryancwalsh commented 2 years ago

Whenever a PR is considered no longer ready for review, we can mark it as a draft like this: https://stackoverflow.com/a/55154745/470749 cc @ozanisgor @norrec99 @rashaabdulrazzak

ozanisgor commented 2 years ago

@ryancwalsh I think the left-align version is looking better. This one was challanging because of responsivenes. It looks good but when you change to small screen logo getting smaller than other items. I tried to figure out but could not find the solution.

I tried to show what i mean by loom video: https://www.loom.com/share/2633635447434b538e4153c951d24367

18 02 22_footer-align-left_after

PREVIOUS versions

18 02 22_footer-align-left_certered-version

17 02 22_layout_after

ryancwalsh commented 2 years ago

@ozanisgor I just made this video for you and then afterwards saw your video here and comments.

https://www.loom.com/share/d728ccf6b9084378a2dac4ae461d89c8

Most of my video is still what I wanted to say, but now that I see your helpful responsiveness demo, I think (unlike what I said in the video) it does make sense to have an inline-block around the social icons.

In the video I'd said that you could optionally have no element to contain them and that they could exist as direct siblings of the other part of the footer.

But I think having them bundled together makes sense. But not acting as a column or with padding.

ozanisgor commented 2 years ago

@ryancwalsh thank you for the video to explain detailed. I hope I understand what you mean. I remove the unnecessary divs and tried to bring all content into one column. While doing this tried to keep responsiveness. I tried to use inline-block but somehow it doesn't work or I coundn't manage to make that work but with flex(according to VSCode they are doing the same) I belive now seems like what you want to see. I tried to explain it with a video and some screenshots below.

I didn't push the code because maybe this not what you want. I can push immedately if you want.

BEFORE

before_footer-align-left

AFTER

after_footer-align-left

AFTER-Centered

after_footer-align-center

AFTER-Small-Devices

after_footer-align-center_small-devices

AFTER-Small-Devices with inline-block

Also wider screens have the same wierd looking.

after_footer-align-center_small-devices_inline-block

VIDEO

When I replay the video I've tnoticed voice is lagging I hope It won't be a problem for you to follow.

https://www.loom.com/share/97cb91d034fb494bba9c6a926b14d8a3

ryancwalsh commented 2 years ago

@ozanisgor The video currently is having a bit of trouble, but I got to watch a lot of it.

You're right that the inline-block behaved in ways that I didn't expect.

I think I saw some good results in the video with what you did. But you didn't push the commits for me to see here :-)

I feel like maybe if you'd pushed the commit of whatever you thought was the best version, I would be accepting it right now.

ryancwalsh commented 2 years ago

@ozanisgor Centered footer looked good.

ozanisgor commented 2 years ago

@ryancwalsh sorry for the problem with the video. I just pushed the code as centered one. I didn't push at the first place because maybe I misunderstood what you want and didn't want to mess.