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

Pages tagged with place get a full-width map across the top of the page (instead of in the sidebar) #5518

Merged
merged 9 commits into from Apr 25, 2019

Conversation

sagarpreet-chadha
Copy link
Contributor

@sagarpreet-chadha sagarpreet-chadha commented Apr 18, 2019

Continued from publiclab/leaflet-blurred-location-display#64 (comment) 😄 !

  • 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

Screenshots :

  • With place tag :

Screenshot 2019-04-18 at 2 53 11 PM

  • Without place tag :

Screenshot 2019-04-18 at 3 04 31 PM

  • human-readable placename is already added as a tag via LBL model :

Screenshot 2019-04-18 at 3 07 28 PM

Thanks!

@plotsbot
Copy link
Collaborator

plotsbot commented Apr 18, 2019

2 Messages
📖 @sagarpreet-chadha 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.
📖 This pull request doesn’t link to a issue number. Please refer to the issue it fixes (if any) in the body of your PR, in the format: Fixes #123.

Generated by 🚫 Danger

@jywarren
Copy link
Member

This looks FANTASTIC!

This has gotten broken a few times... do you think you could write a simple system test for it? #5316 Then we can protect it against future changes? I think it could be pretty easy to add. Thank you!!!

@sagarpreet-chadha
Copy link
Contributor Author

Okay assert_select "div" is not working .
Meaning assert_select does not work in system testing .

@sagarpreet-chadha
Copy link
Contributor Author

Tests passed ❤️ . Ready to be merged .
@jywarren ...kindly review 😄 !!!

@jywarren jywarren merged commit 9a4df9f into master Apr 25, 2019
@jywarren
Copy link
Member

PERFECT awesome.

class PlaceTagsTest < ApplicationSystemTestCase

# node blog has lat and lon tag and place tag .
test "pages tagged with place get a full-width map across the top of the page" do
Copy link
Member

Choose a reason for hiding this comment

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

Love this!!!

Copy link
Member

Choose a reason for hiding this comment

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

Hey, @sagarpreet-chadha can you see if I'm doing anything wrong here? #5605

SrinandanPai pushed a commit to SrinandanPai/plots2 that referenced this pull request May 5, 2019
…age (instead of in the sidebar) (publiclab#5518)

* full width map from sidebar

* indentation done

* code climate issue fixed

* system test added

* system test added

* system test modification

* assert presence of div

* assert_selector used

* tests modified
digitaldina pushed a commit to digitaldina/plots2 that referenced this pull request May 12, 2019
…age (instead of in the sidebar) (publiclab#5518)

* full width map from sidebar

* indentation done

* code climate issue fixed

* system test added

* system test added

* system test modification

* assert presence of div

* assert_selector used

* tests modified
@cesswairimu cesswairimu deleted the feature_place_tag branch May 4, 2021 14:05
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.

None yet

3 participants