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

Show grid of notes with thumbnails #3970

Merged
merged 4 commits into from Nov 15, 2018
Merged

Conversation

kaustubh-nair
Copy link
Member

Fixes #1097
I had to make minor changes in the main notes show html page since the grid was overflowing out of the container for some reason.
Please let me know if there are any changes to be made in the design. I'll add tests soon too.

Here's a screenshot:

plots2

@plotsbot
Copy link
Collaborator

plotsbot commented Nov 14, 2018

2 Messages
📖 @KauNair 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.
📖 Your pull request is on the master branch. Please make a separate feature branch) with a descriptive name like new-blog-design while making PRs in the future.

Generated by 🚫 Danger

Copy link
Member

@jywarren jywarren left a comment

Choose a reason for hiding this comment

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

This is super super exciting; it's been a long-standing issue we are very eager to have solved. THANK YOU SO MUCH!!!!

I've added a note above, where I'd like to see if we could resolve the layout issues you mentioned through a different CSS solution. I'm worried a change of that kind would have to be replicated in other templates for this to work, and I think you've done the hard part of this issue already, so we could potentially work through the layout issue without needing a change to the show.html.erb templates. I may be wrong! But I'd like to give it a try because otherwise we'll have to visually inspect a lot of varying pages that would potentially be affected by the column width changes you're suggesting here. Let's see if we can do it a simpler way.

Thanks again!!! 👍 👍 👍

@@ -1,6 +1,6 @@
<script src="/assets/notes.js" type="text/javascript"></script>

<div class="col-md-9 note-show<% if @node.status == 4 || @node.status == 0 %> moderated<% end %>">
<div class="col-md-12 note-show<% if @node.status == 4 || @node.status == 0 %> moderated<% end %>">
Copy link
Member

Choose a reason for hiding this comment

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

Hm, ok! I see how this may help... however, do you think it could be potentially moving other page content around in a way that would cause trouble? I also wonder what might happen on other pages, such as wiki pages from /app/views/wiki/show.html.erb and /app/views/question/show.html.erb - other places this type of grid might be shown.

Copy link
Member

Choose a reason for hiding this comment

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

er, i guess my note came out below 😄

@kaustubh-nair
Copy link
Member Author

kaustubh-nair commented Nov 15, 2018

Div tags are so difficult to work with. 😞
But I changed the grid into a table, so now it works without having to change the original notes show view. 😄
Also what should I do for notes that do not have any main image?

The thumbnails are slightly bigger than before but I guess that's okay?
plots3

@jywarren
Copy link
Member

This looks great. Any adjustments can be done in follow-up! Thanks so much!!! Again, this was very sought after, so THANKS A LOT!!! 👍 👍 👍

@jywarren jywarren merged commit af82259 into publiclab:master Nov 15, 2018
@jywarren
Copy link
Member

Would you be interested in a new issue, related to this one? Perhaps this one? #3758

@jywarren
Copy link
Member

Also, let's think about what this looks like in mobile phones... can you try in a narrower window? See some about the class-based page layout system here: https://getbootstrap.com/docs/3.3/css/

@kaustubh-nair
Copy link
Member Author

This is how they look on mobile . Maybe we can try responsive tables so they have a scroll bar in them.
Smaller images make the grid irregular. We should scale the all the images to a fixed size so they look better.
Also this tag doesn't have test coverage.
All these should be easy to fix. Do you think we should make new issues for these or should I send a PR?

plots5

@jywarren
Copy link
Member

The Bootstrap classes are responsive, they take some getting used to to use effectively, but are generally worth it. I think we've been able to do this inline; see:

image

See that example here: https://publiclab.org/wiki/micro and click Research

The code is here:

<% if @node.power_tags('tabbed').include?("notes") %>
<div class="tab-pane" id="tab-notes">
<%= render :partial => "notes/notes" %>
<p><a href="/tag/<%= @node.slug_from_path %>"><%= raw t('wiki.show.more_research', :tag => @node.slug_from_path) %> &raquo;</a></p>
<br />
</div>
<% end %>

I think if you want to go for it, a PR is fine. If you'd like to tackle it in stages, start an issue and make a checklist!

I love the idea of test coverage too. Thanks!! Great initiative and great work!

@jywarren
Copy link
Member

Hi, I wanted to note this is live now, and looks great! https://publiclab.org/wiki/micro#Assembly

image

Some of the followup steps will really make this shine. Great work!!!

@vatbq
Copy link
Contributor

vatbq commented Nov 26, 2018

Can someone tell me how render the notes like this? Because when I did notes:tagname it renders a grid, and I don't know how show them with images

@jywarren
Copy link
Member

jywarren commented Nov 26, 2018 via email

@vatbq
Copy link
Contributor

vatbq commented Nov 26, 2018

Yeah. This is merged in master branch, right? I did git pull but there aren't changes, I think that is something related to repository forked. I'm gonna google how to update a fork repository from original

@jywarren
Copy link
Member

if you are in a feature branch, you can do:

  1. git checkout master
  2. git pull https://github.com/publiclab/plots2.git master
  3. git checkout feature-branch
  4. git merge master

Looking for lines of code to help you out too now!

@vatbq
Copy link
Contributor

vatbq commented Nov 26, 2018

Ready! I could do it! Thank you!

@jywarren
Copy link
Member

See how this assembles data in the node_shared file, and then displays them in a template?

https://github.com/publiclab/plots2/pull/3970/files

Hope that helps!

@vatbq
Copy link
Contributor

vatbq commented Nov 26, 2018

Yes, I'm in that 💯

@vatbq
Copy link
Contributor

vatbq commented Nov 26, 2018

Hey @jywarren I could do something like that
screen shot 2018-11-26 at 8 06 12 pm

What do you think?

@jywarren
Copy link
Member

jywarren commented Nov 27, 2018 via email

@vatbq
Copy link
Contributor

vatbq commented Nov 27, 2018

Thank you! The maximum number of images per row is 3, if that is that you refer.
Do you want that I add a link in every note?

@jywarren
Copy link
Member

jywarren commented Nov 27, 2018 via email

@vatbq
Copy link
Contributor

vatbq commented Nov 27, 2018

Oh, I understand. I'm trying to do it

@vatbq
Copy link
Contributor

vatbq commented Nov 27, 2018

Me again! Well, I did it
more
Also, I did a show less button but I didn't have time to recording because github just allow 10MB

SrinandanPai pushed a commit to SrinandanPai/plots2 that referenced this pull request May 5, 2019
* add nodeshared module for thumbnails

* add title to notes thumbnail

* change grid to table

* remove unnecessary file
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants