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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

horizontal cards for people section #5740

Merged
merged 1 commit into from May 21, 2019

Conversation

@CleverFool77
Copy link
Member

commented May 19, 2019

Fixes #5707 part

User Desktop

Screenshot from 2019-05-19 17-43-02

User mobile

Screenshot from 2019-05-19 17-44-23

Admin Desktop

Screenshot from 2019-05-19 17-58-25

Admin Mobile

Screenshot from 2019-05-19 17-58-52

  • PR is descriptively titled 馃搼 and links the original issue above 馃敆
  • tests pass -- look for a green checkbox 鉁旓笍 a few minutes after opening your PR -- or run tests locally with rake test
  • code is in uniquely-named feature branch and has no merge conflicts 馃搧
  • screenshots/GIFs are attached 馃搸 in case of UI updation
  • ask @publiclab/reviewers for help, in a comment below

@CleverFool77 CleverFool77 force-pushed the CleverFool77:horizontal branch from 5ecd409 to e14e657 May 19, 2019

@CleverFool77

This comment has been minimized.

Copy link
Member Author

commented May 19, 2019

Hi @jywarren @gauravano @gautamig54
Now that we are beginning the work.
I started off with changing the people section user cards to horizontal.

@plotsbot

This comment has been minimized.

Copy link
Collaborator

commented May 19, 2019

2 Messages
馃摉 @CleverFool77 Thank you for your pull request! I鈥檓 here to help with some tips and recommendations. Please take a look at the list provided and help us review and accept your contribution! And don鈥檛 be discouraged if you see errors 鈥 we鈥檙e here to help.
馃摉 This pull request doesn鈥檛 link to a issue number. Please refer to the issue it fixes (if any) in the body of your PR, in the format: Fixes #123.

Generated by 馃毇 Danger

@CleverFool77

This comment has been minimized.

Copy link
Member Author

commented May 19, 2019

The failing test of dashboard doesn't seem to be related to this. I guess.
Thanks !!!!

1 similar comment
@CleverFool77

This comment has been minimized.

Copy link
Member Author

commented May 19, 2019

The failing test of dashboard doesn't seem to be related to this. I guess.
Thanks !!!!

@sagarpreet-chadha
Copy link
Contributor

left a comment

The changes are great 馃挴 !!!
Do you think we should update/remove the CSS of /peopleleaflet here ?

#map<%= unique_id %> { width:100%; height:500px; margin: 0; position: relative;}

@sagarpreet-chadha

This comment has been minimized.

Copy link
Contributor

commented May 20, 2019

Agreed the failing test is not due to changes in this PR . Lets start the build again on this PR .

@sagarpreet-chadha

This comment has been minimized.

Copy link
Contributor

commented May 20, 2019

Hi @jywarren ,
How can we see the screenshot tmp/screenshots/test_viewing_the_dashboard.png ?
PS : After reopening the PR , now there are 2 system tests failing instead of 1 initially .

@CleverFool77

This comment has been minimized.

Copy link
Member Author

commented May 20, 2019

The changes are great !!!
Do you think we should update/remove the CSS of /peopleleaflet here ?

#map<%= unique_id %> { width:100%; height:500px; margin: 0; position: relative;}

Hi @sagarpreet-chadha
May I know what change you are mentioning about ?
If it's about width and height , then they are required.
Maybe we can look into the matter of margin ,position and remove it.
Thanks !!!

@sagarpreet-chadha

This comment has been minimized.

Copy link
Contributor

commented May 20, 2019

Just noting a couple of things :

  1. When we scroll-down , should the map also move downwards ? I think we have plenty of space in that column to do that , currently it kinds of feel like lots of empty space .

  2. In mobile view , i think the map should come at the top (currently it goes at the bottom , right ?) .

I would love to hear your thoughts @CleverFool77 , @jywarren .

@jywarren

This comment has been minimized.

Copy link
Contributor

commented May 20, 2019

@CleverFool77

This comment has been minimized.

Copy link
Member Author

commented May 20, 2019

Hi @jywarren @sagarpreet-chadha I'll fix the map position and other features in next PR.
Regarding moving map downwards, the features of making map sticky is already in discussion I guess.
For now, Is there any other changes required in this PR?
Thanks!!

@gautamig54

This comment has been minimized.

Copy link
Contributor

commented May 21, 2019

This looks great @CleverFool77.
Regarding making the map sticky and positioning it on top in mobile view, I'll add them to the checklist and implement them in the next PR. Thanks!

@gautamig54

This comment has been minimized.

Copy link
Contributor

commented May 21, 2019

I would like to mention one thing, as per the design, there is a decrease in the left and right margin of the people's page, due which the width of the cards has be increased, resulting in a cleaner card.
What do you think about this @jywarren @CleverFool77 ?

@CleverFool77

This comment has been minimized.

Copy link
Member Author

commented May 21, 2019

Hi @gautamig54 regarding side margins you are talking about. I guess it's being followed everywhere in new design due to bootstrap4 container. You can notice it in every page whether it's dashboard or other.
Thanks !!!

@CleverFool77 CleverFool77 force-pushed the CleverFool77:horizontal branch from e14e657 to 4226ac2 May 21, 2019

@CleverFool77

This comment has been minimized.

Copy link
Member Author

commented May 21, 2019

Just Fixed map margins and Updated the PR:
Screenshot from 2019-05-21 16-30-09

@jywarren

This comment has been minimized.

Copy link
Contributor

commented May 21, 2019

@gautamig54

This comment has been minimized.

Copy link
Contributor

commented May 21, 2019

It looks great @CleverFool77.
This is ready to be merged @jywarren
Thanks!

@CleverFool77

This comment has been minimized.

Copy link
Member Author

commented May 21, 2019

yes @jywarren It's ready for merge.
Thanks !!!
cc : @gautamig54

@gautamig54

This comment has been minimized.

Copy link
Contributor

commented May 21, 2019

I am ready with adding "add your location" button. Since, you have fixed map margins, I will push a PR, once this PR gets merged.

@jywarren jywarren merged commit d7fcbc5 into publiclab:master May 21, 2019

3 checks passed

codeclimate All good!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
danger/danger All green. Jolly good show.
Details
@jywarren

This comment has been minimized.

Copy link
Contributor

commented May 21, 2019

Awesome!!!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
5 participants
You can鈥檛 perform that action at this time.