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

html cards for people section #5169

Merged
merged 1 commit into from
May 9, 2019
Merged

Conversation

CleverFool77
Copy link
Member

@CleverFool77 CleverFool77 commented Mar 19, 2019

Fixes #4922 (<=== Add issue number here)

Earlier Description

People page will be redesigned soon. And one the requirements was that all the users should be listed down in grid card format instead rows in table. I started off with the basic structure of cards for users. And soon the further improvements will be done.

Screenshot from 2019-04-03 19-34-10

h

Update

This is the new redesign of people's page.
I made it two column and map on right. The map remain stick on scrolling.
I showed the users on left
On md and lg scrrens -
h

On small screens -
hh

  • 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

Thanks!

@CleverFool77 CleverFool77 force-pushed the card-html branch 3 times, most recently from 56fb2f6 to 4e5f061 Compare March 19, 2019 05:44
@plotsbot
Copy link
Collaborator

plotsbot commented Mar 19, 2019

1 Message
📖 @CleverFool77 Thank you for your pull request! I’m 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’t be discouraged if you see errors – we’re here to help.

Generated by 🚫 Danger

@CleverFool77
Copy link
Member Author

@gauravano i tried the following to pass the tests
since now there is no static value in header to check for existence, i tried asserting usernames of user but i wasn't able to pass the tests. I fetched the users and stored in a variables user_data = users(). I asserted as user_data.username().to_s inside a loop. but the issue was the user in this way appeared as <spammer>
Then i tried checking for the existence of the row in which all cards are going. I gave that div an id but I wasn't able to find how to check existence of a div with specific id in ruby test.

How can I proceed to pass the tests.

@jywarren
Copy link
Member

Hi, going to try to help with the errors too, but also wanted to say - this is very exciting! Thanks!

I wonder if we can continue refining the card display even more; for example:

image

Could we:

  1. add a bit more padding/margins inside so that it matches the mockup a bit more?
  2. make the username text black and a larger size, and Junction font?
  3. make the rest of the text smaller and grey, with a bit more line spacing?

This is very cool! Its going to be awesome!

@jywarren
Copy link
Member

Ok, do you think you could push up the tests you've written so far? Here are a few examples of some CSS stuff you can assert_select, but it's not quite as flexible as jQuery in fetching specific CSS:

https://github.com/publiclab/plots2/blob/master/test/functional/notes_controller_test.rb#L56-L57

https://github.com/publiclab/plots2/blob/master/test/functional/notes_controller_test.rb#L157-L158

I hope this helps!

@CleverFool77
Copy link
Member Author

CleverFool77 commented Mar 23, 2019

Hi @jywarren. Thanks for the help. I'll follow the methods in links. Regarding the cards,I was thinking of making basic layout first and then refining them but now that you've mentioned I'll make them look more like the ui in the design given.
Thanks for the review :)

@CleverFool77
Copy link
Member Author

CleverFool77 commented Apr 3, 2019

Hi @jywarren and @gauravano
I updated this PR.
I redesigned the basic structure of the people page
I was confused regarding the card ui that which one to choose, considering there were two designs made by team.
s
and
ss

For now, I went for the ui of card in the above comments.
I would like to have your views regarding the card design from both of them.
I removed the test for I18n for 'users.list.members_last_activity , as the data to be tested has been removed.
Regarding the functional test, I'll write them to check card rendering.

@grvsachdeva
Copy link
Member

Lol! both of them are awesome! I can't choose one 🙈. Let's ask @publiclab/community-reps

@grvsachdeva
Copy link
Member

Although, have you implemented 2nd one on the website, yet?

@CleverFool77
Copy link
Member Author

Hi @gauravano
I implemented second one as warren comment in above comments to make the ui similar to it. But Now if I'll go for grid system of two column with map in right side. I'm not sure which UI is chosen.

@CleverFool77
Copy link
Member Author

CleverFool77 commented Apr 4, 2019

Hi again @jywarren @gauravano ,
The update in the PR description is latest changes I've pushed.
I didn't remove the Earlier description as I needed two choices for comparison.

@CleverFool77 CleverFool77 force-pushed the card-html branch 6 times, most recently from e326116 to a40a00e Compare April 5, 2019 14:16
@jywarren
Copy link
Member

jywarren commented Apr 5, 2019

This is looking amazing! However, I maybe want to caution that it could be better to work in smaller pieces -- you've done some great work here, but while we could have merged the card UI now, we should take a few other steps on the other parts, before merging. So, since it's all in one PR, we will need to hold a little bit. Doing it in smaller parts is also great for sharing the work among people! But that's OK this time, just a note for next time! 🙌 👍 🎉

Note a few things here. The UI you've done is super. Now take a look at this screenshot of the existing:

image

What we should think about is:

  1. the table sorting. Can we preserve it as was done on this page? Sort option is changed according to the new UI #5239
  2. there is a "ban" button shown to admins/moderators. Could this be put after 'joined ____ ago' for now, until we do a follow-up PR (modularity in mind 😄 ) for the ... menu?

What do you think?

Thank you @CleverFool77 !!! This is very cool!

@CleverFool77
Copy link
Member Author

CleverFool77 commented Apr 6, 2019

Hi @jywarren I started off with basic card-layout in this PR but as I proceeded to make the card look like the provided ui by team. It itself turned into this.
Regarding sorting and other, It was left as next part. Or should the sorting part be done here only ?
regarding the ban button, it's there I'll provide the gif after logging as admin. As right now In the gif I uploaded, I logged in as new contributor.
Tbh earlier my aim for this PR was a baisc card layout but as I proceeded, it turned into something major.

Thanks !!!!!

@CleverFool77
Copy link
Member Author

CleverFool77 commented Apr 6, 2019

hi @jywarren @gauravano
I did some fixes to size of ban button
And here is the screenshot
Desktop
Screenshot from 2019-04-06 11-34-59

Mobile interface
Screenshot from 2019-04-06 11-35-23

@CleverFool77
Copy link
Member Author

Hi @jywarren @gauravano ,
Here is the latest changes that I've comitted.

Admin
dd

User
hh

@jywarren
Copy link
Member

jywarren commented Apr 8, 2019

This looks really good!!! I am wondering, if it's possible to wait until after we merge the giant #3937 Bootstrap 4 upgrade, and then be sure this works on Bootstrap 4 styles too? What do you think?

We're trying to all collaborate on identifying the final issues with the upgrade in this issue: #5182

@SidharthBansal
Copy link
Member

I think we should merge #3937 first.

@jywarren
Copy link
Member

OK! #3937 was merged!!!!! @CleverFool77 would you want to try updating over that change and ensuring Bootstrap 4 compatibility? Thank you!!!

@CleverFool77
Copy link
Member Author

CleverFool77 commented Apr 24, 2019

Hi @jywarren
I'll update it soon as my exams are going on right now.
thanks !!

@SidharthBansal
Copy link
Member

No rush!!! Best of luck for your exams

@SidharthBansal
Copy link
Member

There are some conflicts in your pr. It will be great if you can rebase your pr whenever you are free.
Thanks for the work done here at PL

@CleverFool77 CleverFool77 reopened this May 9, 2019
@CleverFool77
Copy link
Member Author

Hi,
I resolved few stuffs here and did changes as per bootstrap4 upgrade.
I need some review regarding the changes.
@jywarren @SidharthBansal @gauravano
Thanks !!!

@jywarren
Copy link
Member

jywarren commented May 9, 2019

This is looking great. Can you share a screenshot? Thanks!!

@jywarren jywarren closed this May 9, 2019
@jywarren jywarren reopened this May 9, 2019
@CleverFool77
Copy link
Member Author

CleverFool77 commented May 9, 2019

Hi @jywarren
I need some suggestions regarding following points -

  • The side margins. It should be decreased ?
  • The cards look elongated vertically for admin due to ban button. Should I work on font ? or size ? or margin ? Though this issue isn't there in user side.
  • Make map sticky ?

people

Admin

Screenshot from 2019-05-09 21-37-12

User

Screenshot from 2019-05-09 21-44-54

@CleverFool77 CleverFool77 force-pushed the card-html branch 4 times, most recently from a17f7ec to 6f4d53a Compare May 9, 2019 16:06
@jywarren
Copy link
Member

jywarren commented May 9, 2019 via email

@CleverFool77
Copy link
Member Author

Hi @jywarren
I went through style guide link.
The Design seems bit different. It's not two equal column anymore. And few other changes.
And regarding the card too. There were two card design. On the basis of conversation happened above in this PR. I started working on vertical one. So Should I go for horizontal one ?
Should I do it like as it's mentioned in style guide ? or should I just wait for sometime as there would be many more suggestions and changes in style guide, I guess.

@jywarren
Copy link
Member

jywarren commented May 9, 2019 via email

@CleverFool77
Copy link
Member Author

CleverFool77 commented May 9, 2019

I decreased the padding for card-info body by 0.5em on left and right. For bottom and top, I decreased it by 0.7em and 1em.
I've updated the PR.
Thanks !!!

Screenshot from 2019-05-09 23-39-10

@jywarren
Copy link
Member

jywarren commented May 9, 2019

This looks great! I'll merge this once it passes. Thanks a lot, @CleverFool77 !!!

@jywarren jywarren merged commit ef86cca into publiclab:master May 9, 2019
digitaldina pushed a commit to digitaldina/plots2 that referenced this pull request May 12, 2019
@CleverFool77 CleverFool77 deleted the card-html branch June 28, 2019 17:47
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.

Planning implementation of new UI designs (work in progress!)
5 participants