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

Already on GitHub? Sign in to your account

Jump to bottom

Toggle view option for image added in the sidebar #738

Merged
merged 3 commits into from
Aug 16, 2019
Merged

Toggle view option for image added in the sidebar #738

merged 3 commits into from
Aug 16, 2019

Conversation

divyabaid16
Copy link
Contributor

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

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

GIF:
Peek 2019-06-24 18-15

  • 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/mapknitter-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!

@codeclimate
Copy link

codeclimate bot commented Jun 24, 2019

Code Climate has analyzed commit 7602cab6 and detected 0 issues on this pull request.

View more on Code Climate.

@divyabaid16
Copy link
Contributor Author

@publiclab/mapknitter-reviewers

@divyabaid16
Copy link
Contributor Author

All the tests have passed here.
Thanks!

@kaustubh-nair
Copy link
Member

Looks good to me 馃憤

@divyabaid16 divyabaid16 mentioned this pull request Jun 26, 2019
Closed
@divyabaid16 divyabaid16 mentioned this pull request Jul 2, 2019
Open
5 tasks
@grvsachdeva
Copy link
Member

Hi @divyabaid16, looks great. Please remove the merge conflicts.

As a follow-up, we can improve the design from button to icons? Also, how about showing image options in grid view too? @publiclab/mapknitter-reviewers do you think it would look good?

Thank you.

@codecov
Copy link

codecov bot commented Jul 28, 2019 •
edited
Loading

Codecov Report

Merging #738 into main will increase coverage by 0.36%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #738      +/-   ##
==========================================
+ Coverage   72.78%   73.15%   +0.36%     
==========================================
  Files          37       37              
  Lines        1356     1356              
==========================================
+ Hits          987      992       +5     
+ Misses        369      364       -5
Impacted Files Coverage 螖
lib/exporter.rb 94.17% <0%> (+2.24%) 猬嗭笍

@divyabaid16
Copy link
Contributor Author

@gauravano I have rebased my PR.
Thanks!

@grvsachdeva
Copy link
Member

Hey @divyabaid16, CSS is not working well for images of different size

Screenshot from 2019-07-28 22-54-45

Could you try to limit all the columns of sidebar in a single row? Thank you!

@grvsachdeva grvsachdeva added this to In progress in Image Management via automation Jul 28, 2019
@divyabaid16
Copy link
Contributor Author

List view is now working fine @gauravano .

@divyabaid16
Copy link
Contributor Author

I'm not sure why the test is failing here. Can anyone tell me the reason for this? @publiclab/mapknitter-reviewers

@cesswairimu
Copy link
Collaborator

Hey @divyabaid16, CodeCov is misbehaving a lot on PRs...Could you try rebasing with main? this might fix it.

@divyabaid16
Copy link
Contributor Author

Hmm, it failed even after rebasing.

@cesswairimu
Copy link
Collaborator

CodeCov expects us to add tests though you did not change any functionality 馃

@divyabaid16
Copy link
Contributor Author

Should we merge this even though the test is failing?

@jywarren
Copy link
Member

Let's ignore CodeCov here for now, we're working on that elsewhere... can you resolve conflicts and then we should be good? Thanks!

@divyabaid16
Copy link
Contributor Author

@jywarren I have resolved the conflicts. This is good to go.
Thanks!

@jywarren jywarren merged commit c7d700d into publiclab:main Aug 16, 2019
Image Management automation moved this from In progress to Done Aug 16, 2019
@jywarren
Copy link
Member

Nice!!! Great work!

@divyabaid16 divyabaid16 mentioned this pull request Aug 25, 2019
Merged
5 tasks
chen-robert pushed a commit to chen-robert/mapknitter that referenced this pull request Dec 5, 2019
@divyabaid16 @chen-robert
Toggle view option for image added in the sidebar (publiclab#738)
aa48fc0 
* Toggle option for image view added

* list view correction

* Toggle-view
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@grvsachdeva grvsachdeva grvsachdeva left review comments

@kaustubh-nair kaustubh-nair kaustubh-nair approved these changes

@jywarren jywarren Awaiting requested review from jywarren

Assignees
No one assigned
Labels
None yet
Projects
Image Management
  
Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add toggle view for images in the sidebar
5 participants
@divyabaid16 @kaustubh-nair @grvsachdeva @cesswairimu @jywarren