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.

Sign up for GitHub

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

Jump to bottom

sidebar tags card #6124

Closed
wants to merge 11 commits into from
Closed

sidebar tags card #6124

wants to merge 11 commits into from

Conversation

gautamig54
Copy link
Contributor

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

Make sure these boxes are checked before your pull request (PR) is ready to be reviewed and merged. Thanks!

  • 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

We're happy to help you get this ready -- don't be afraid to ask for help, and don't be discouraged if your tests fail at first!

If tests do fail, click on the red X to learn why by reading the logs.

Please be sure you've reviewed our contribution guidelines at https://publiclab.org/contributing-to-public-lab-software

Thanks!

@gautamig54
Copy link
Contributor Author

Screenshots are :

Screen Shot 2019-08-09 at 9 37 51 PM

Screen Shot 2019-08-09 at 9 38 33 PM

Screen Shot 2019-08-09 at 9 38 49 PM

The plus sign is the toggle button which opens up the form for tag

@gautamig54
Copy link
Contributor Author

I am implementing the limit to display only 2 tags at a time and display the rest with clicked on "#more".
Is the design here alright @CleverFool77 @gauravano ??

@gautamig54 gautamig54 closed this Aug 9, 2019
@gautamig54 gautamig54 reopened this Aug 9, 2019
@grvsachdeva
Copy link
Member

The + button looks weird at the bottom of wiki page sidebar. Can we shift it to the top with other buttons and we can focus on the tags field when user clicks on it? Or, if you have any design in mind?

And, yes, showing lesser tags would be great? Let's have 2 tags for now. Thank you!

@CleverFool77
Copy link
Member

The button on the design draft looks softer 🤔

@gautamig54
Copy link
Contributor Author

The button on the design draft looks softer 🤔

softer as in?

The + button looks weird at the bottom of wiki page sidebar. Can we shift it to the top with other buttons and we can focus on the tags field when user clicks on it? Or, if you have any design in mind?

And, yes, showing lesser tags would be great? Let's have 2 tags for now. Thank you!

Yes the pkus sign will go up and I'll be showing just 2 tags at the beginning.

@gautamig54
Copy link
Contributor Author

This is the current design.

Screen Shot 2019-08-13 at 2 29 54 PM

@jywarren
Copy link
Member

jywarren commented Aug 14, 2019 via email

@jywarren
Copy link
Member

Actually can't we use the "card" classes from Bootstrap for this box, just like we do on tag cards? https://publiclab.org/tags That'd be nice!

@gautamig54 gautamig54 closed this Aug 17, 2019
@gautamig54 gautamig54 reopened this Aug 17, 2019
@gautamig54
Copy link
Contributor Author

Hi @jywarren!
I have made the said changes. This PR is now ready.

Screen Shot 2019-08-18 at 3 12 05 PM
Screen Shot 2019-08-18 at 3 12 16 PM

@gautamig54 gautamig54 changed the title [WIP] sidebar tags card sidebar tags card Aug 18, 2019
Anupam-dagar
Anupam-dagar suggested changes Aug 18, 2019
View reviewed changes
Copy link
Member

@Anupam-dagar Anupam-dagar left a comment

Choose a reason for hiding this comment

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

Hi @gautamig54, I left some comments.

app/views/tag/_tagging.html.erb Outdated
}
</style>

<style>
Copy link
Member

Choose a reason for hiding this comment

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

Please shift all css in a single style tag. This will make the page little lighter in size.

Anupam-dagar
Anupam-dagar suggested changes Aug 18, 2019
View reviewed changes
Copy link
Member

@Anupam-dagar Anupam-dagar left a comment •
edited
Loading

Choose a reason for hiding this comment

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

@gautamig54 Also for the smaller devices, the design is breaking. Attached below is the screenshot.
To keep the design consistent, I suggest shifting x people under the tag name for every card. For smaller devices the ellipsis menu ... is shifting to another line and not coming properly. Please fix it.
Also, I feel that the information displayed on clicking the ellipsis is coming way lower than it should. There is quite a distance between the ellipsis and the detail shown.
Finally, I observed that if there are more than 2 tags then along side the + button,x more appears. But on clicking it nothing happens, I believe it should show the remaining tags?

ellpsis detail much lower
Screenshot from 2019-08-18 16-46-27

1 more tag and inconsistency in ellipsis position.
Screenshot from 2019-08-18 16-44-49

x people inconsitency
Screenshot from 2019-08-18 16-51-26

@gautamig54
Copy link
Contributor Author

Thanks @Anupam-dagar for the review. I'll make the suggested changes :)
Regarding the x people consistency, I think it is okay. It will shift when the tag name is too long to fit in single line. I'll correct the responsiveness of the sidebar.
Thanks!

@Anupam-dagar
Copy link
Member

Anupam-dagar commented Aug 18, 2019 •
edited
Loading

But for smaller names, they will appear in different lines. Same line for small name and different line for bigger name. This leads to a bad UI. I will leave this for further discussion from more involved contributors.
You can see the inconsistency in the screenshots above itself for the same names.
Please fix the travis build too.

@jywarren
Copy link
Member

Hi @gautamig54 -- do you think we can use the same Bootstrap card class as the tag cards? I think that'd be nice for consistency! Thank you!

@gautamig54
Copy link
Contributor Author

I have fixed the minor changes and have added the card class as well for consistency.

@gautamig54
Copy link
Contributor Author

The test fails due to some errors in profile page. @CleverFool77 can you look into this?
Other than that, this PR is ready for merge.

@jywarren
Copy link
Member

Ah, looks like a few different tests are failing!

image

They are looking good but I'm not sure if the new screenshots were uploaded with the new CSS changes:

image

Maybe it failed in system tests and didn't pick up the new changes yet...

@gautamig54
Copy link
Contributor Author

This is the screenshot of the latest version of the PR.

Screen Shot 2019-08-26 at 10 48 01 PM

@jywarren
Copy link
Member

jywarren commented Aug 26, 2019 via email

@jywarren
Copy link
Member

Hi @gautamig54 -- would you look through the tests and copy the errors into here, and see if you can go through them one by one to resolve them? Thank you so much!

@jywarren jywarren mentioned this pull request Aug 27, 2019
Merged
@CleverFool77
Copy link
Member

Hi @gautamig54 I'm restarting the build and checking it for tests.

@CleverFool77 CleverFool77 reopened this Aug 27, 2019
@CleverFool77
Copy link
Member

System test failure is not related. Checking it for functional and Integration testing.

@CleverFool77
Copy link
Member

CleverFool77 commented Aug 27, 2019 •
edited
Loading

Hi @gautamig54 I see what's the problem is. The reset key and ban button in the profile page is not visible . And that's where we are getting the error.
Actually the problem is, In the new the profile page, we have hidden these buttons behind the ... icon. So only when clicked, we can see these buttons. And so in these functional testing. It's unable to find these buttons in frontend UI tetsing.

System test failure is not related.

@jywarren
Copy link
Member

Hi @gautamig54, do you have any updates here? This is so close, let's just wrap the last couple things and get it merged! Thank you!

@gautamig54
Copy link
Contributor Author

Hi @jywarren! I am not sure if the test failures are related to this issue. Can we merge this?

@jywarren
Copy link
Member

jywarren commented Aug 31, 2019 via email

@gautamig54
Copy link
Contributor Author

gautamig54 commented Aug 31, 2019 via email

@gautamig54
Copy link
Contributor Author

gautamig54 commented Aug 31, 2019 via email

@jywarren
Copy link
Member

jywarren commented Sep 4, 2019 via email

jywarren
jywarren reviewed Sep 11, 2019
View reviewed changes
app/views/tag/_tags.html.erb
<% end %>

<% elsif tag.class == UserTag && (tag.name[0..4] != "oauth" || (logged_in_as(['admin', 'moderator']) || (current_user && current_user.uid == tag.uid))) %>
<% tags.limit(2).each do |tag| %>
Copy link
Member

Choose a reason for hiding this comment

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

Here, I think this may cause some trouble - it looks like we would need to switch by tag.class outside this iterator, so profile tags are unaffected. Then, we can split off the first 2, within the NodeTag block, and show those as cards. The rest we can show, still in that block, the previous way.

@jywarren
Copy link
Member

The styling looks good here! Now, we just need to rework the logic around profile tags vs. node tags.

image

@jywarren
Copy link
Member

OK, I worked on this a good bit to get it to render well with many different combinations of tags, different screen sizes, etc. I hope this gets most of the edge cases, but it's not perfect... we may need to do some follow-up tweaks. In any case, here's what I'm seeing locally:

Mobile width initially:

image

And expanded:

image

Full width initially:

image

And expanded:

image

@jywarren jywarren mentioned this pull request Sep 22, 2019
Merged
@jywarren
Copy link
Member

Hm, I couldn't push here, so I'm opening a new PR! Moving to: #6315

Thanks all! This was great work. Glad to be able to finalize and merge it! 👍

@jywarren jywarren closed this Sep 22, 2019
@jywarren jywarren mentioned this pull request Sep 23, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jywarren jywarren jywarren left review comments

@Anupam-dagar Anupam-dagar Anupam-dagar requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

New sidebar design implementation for the notes show page
5 participants
@gautamig54 @grvsachdeva @CleverFool77 @jywarren @Anupam-dagar