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

New view only map page #777

Merged
merged 23 commits into from
Sep 9, 2019
Merged

New view only map page #777

merged 23 commits into from
Sep 9, 2019

Conversation

cesswairimu
Copy link
Collaborator

@cesswairimu cesswairimu commented Jul 1, 2019 •
edited
Loading

Fixes #761

  • 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

codeclimate[bot]
codeclimate bot reviewed Jul 1, 2019
View reviewed changes
app/controllers/front_ui_controller.rb Outdated
@@ -40,6 +40,14 @@ def save_location
render nothing: true
end

def view_map
# @map = Map.find_by(slug: params[:id])
@map= Map.last
Copy link

Choose a reason for hiding this comment

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

Surrounding space missing for operator =.

@codeclimate
Copy link

codeclimate bot commented Jul 1, 2019

Code Climate has analyzed commit 7b0fbc18 and detected 1 issue on this pull request.

Here's the issue category breakdown:

Category Count
Style 1

View more on Code Climate.

@cesswairimu cesswairimu changed the title [WIP] new view only map page [WIP]New view only map page Jul 1, 2019
@codecov
Copy link

codecov bot commented Jul 1, 2019 •
edited
Loading

Codecov Report

Merging #777 into main will increase coverage by 0.22%.
The diff coverage is 66.66%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #777      +/-   ##
==========================================
+ Coverage   73.26%   73.48%   +0.22%     
==========================================
  Files          40       40              
  Lines        1384     1388       +4     
==========================================
+ Hits         1014     1020       +6     
+ Misses        370      368       -2
Impacted Files Coverage Δ
app/controllers/maps_controller.rb 89.56% <66.66%> (-2.33%) ⬇️
lib/exporter.rb 94.17% <0%> (+2.24%) ⬆️

@jywarren
Copy link
Member

jywarren commented Jul 2, 2019

Exciting!!!

@jywarren
Copy link
Member

jywarren commented Jul 3, 2019

I was thinking, when we finish this, maybe we should apply it to the tag page too, /tag/featured for example! With a map of the maps with that tag.

@cesswairimu
Copy link
Collaborator Author

@jywarren noted..Will work to get this done today

@cesswairimu cesswairimu force-pushed the view-only-page branch 3 times, most recently from bce8925 to d024451 Compare July 5, 2019 20:13
@cesswairimu
Copy link
Collaborator Author

Hey @jywarren , what do u think? Also where do we want the edit this map button to link to/do?

screencapture-localhost-3000-m-indiana-2019-07-06-01_38_26

@jywarren
Copy link
Member

jywarren commented Jul 8, 2019

Looking great! I think:

  1. "env data near here" will have to wait until LEL's menu is done - see https://github.com/publiclab/leaflet-environmental-layers/milestone/2 and Make dynamic demo using static html table menu design leaflet-environmental-layers#232
  2. can we add "Nearby mappers" or should we really show "nearby maps" and just highlight who the mappers are?
  3. "Edit this map" will go to the current page like https://mapknitter.org/maps/pvdtest/ where you can actually edit the map! make sense?

@jywarren
Copy link
Member

jywarren commented Jul 8, 2019

What if we had a section like "Share + Embed" or something?

@cesswairimu
Copy link
Collaborator Author

Great thanks Jeff

@cesswairimu
Copy link
Collaborator Author

I think displaying nearby maps and highlighting the mappers is seems more convenient...I will change to that.

@cesswairimu cesswairimu force-pushed the view-only-page branch 4 times, most recently from 926abc2 to eddb8de Compare July 9, 2019 20:27
jywarren
jywarren reviewed Jul 9, 2019
View reviewed changes
app/controllers/front_ui_controller.rb Outdated
@@ -38,6 +38,14 @@ def save_location
render nothing: true
end

def view_map
Copy link
Member

Choose a reason for hiding this comment

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

Oh, maybe this belongs in the maps_controller, next to the show method? Thanks, Cess!

@cesswairimu
Copy link
Collaborator Author

screencapture-mapknitter-unstable-laboratoriopublico-org-m-central-park-nyc-2019-07-10-01_02_32

@cesswairimu
Copy link
Collaborator Author

Hey @jywarren could you please point me to the partial/script that renders the images on top of the map? Thanks

@jywarren jywarren mentioned this pull request Jul 10, 2019
Open
@jywarren
Copy link
Member

jywarren commented Jul 10, 2019 via email

@cesswairimu
Copy link
Collaborator Author

Great thanks

@cesswairimu
Copy link
Collaborator Author

cesswairimu commented Jul 11, 2019 •
edited
Loading

Hey @jywarren, I removed the 'Env data near here' section until LEL is ready. I also moved the logic to maps_controller. I added an embed code button..let me know if this is what you had in mind?
screencapture-localhost-3000-m-nairobi-2019-07-11-13_51_43

Also thinking for a follow-up issue maybe we can change the map design in Map.js to the same map design as we have for the new front-page or a another design what do you think?

@cesswairimu cesswairimu changed the title [WIP]New view only map page New view only map page Jul 13, 2019
@cesswairimu
Copy link
Collaborator Author

Hey @jywarren , maybe we could merge this now that rails 5 is working fine? Thanks

@jywarren
Copy link
Member

jywarren commented Sep 9, 2019

Hi Cess! Yes, sounds good. Merging now, can you check it out on stable once we do?

@jywarren jywarren merged commit a6413bb into publiclab:main Sep 9, 2019
@sashadev-sky sashadev-sky mentioned this pull request Sep 10, 2019
Merged
5 tasks
@cesswairimu cesswairimu mentioned this pull request Sep 10, 2019
Closed
5 tasks
@sashadev-sky sashadev-sky mentioned this pull request Sep 10, 2019
Merged
7 tasks
@cesswairimu
Copy link
Collaborator Author

Hey Jeff, it is working fine on stable apart from the routing error that Sasha has fixed in her latest PR. Thanks

This was referenced Sep 10, 2019
Closed
Merged
@sashadev-sky sashadev-sky mentioned this pull request Sep 11, 2019
Merged
5 tasks
chen-robert pushed a commit to chen-robert/mapknitter that referenced this pull request Dec 5, 2019
@cesswairimu @chen-robert
New view only map page (publiclab#777)
e48dd43 
* view map page

* view map routing and styling

* improve map view

* style mappers

* add edit map button

* test  view map action

* Enhance view page

* show nearby maps

* change div to class

* update tests

* move view_map to maps controller

* show images on top of map

* fix filename typo

* fix typo

* Fix mobile view

* include tags in posting to publiclab

* fix styling

* change to new view page and create an edit page

* add show page button in edit

* fix failing tests

* make buttons style consistent

* change button to exit editor

* remove observation section
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jywarren jywarren jywarren requested changes

@grvsachdeva grvsachdeva grvsachdeva left review comments

@codeclimate codeclimate[bot] codeclimate[bot] left review comments

@sashadev-sky sashadev-sky sashadev-sky left review comments

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

Successfully merging this pull request may close these issues.

New "view only" map page
4 participants
@cesswairimu @jywarren @grvsachdeva @sashadev-sky