Skip to content

Conversation

@AngelaGuardia
Copy link
Contributor

@AngelaGuardia AngelaGuardia commented Jan 31, 2021

Why

Improving user privacy by removing person name from the contributions map view
Addresses one item in #819

Pre-Merge Checklist

  • All new features have been described in the pull request
  • Security & accessibility have been considered
  • High quality tests have been added, or an explanation has been given why the features cannot be tested
  • New features have been documented, and the code is understandable and well commented
  • Entry added to CHANGELOG.md if appropriate
  • All outstanding questions and concerns have been resolved
  • Any next steps that seem like good ideas have been created as issues for future discussion & implementation

What

  • prevent person name from showing on map view

Before
Screen Shot 2021-02-03 at 1 54 18 PM

After
Screen Shot 2021-02-03 at 1 59 40 PM

How

  • Removed (person.name) from listing.rb name method

Testing

  • No tests were added for this. Tested visually.

Next Steps

Outstanding Questions, Concerns and Other Notes

  • The current solution may affect other views in the app where the person name is desired
  • Instead of modifying the name method, I wanted to create a new method in listing.rb called private_name and call that on line 63 of MapBrowser.vue. That way I could be certain that this change would not affect other areas of the app. I wasn't able to make that work but it may be a better solution.

Accessibility

Security

Meta

@AngelaGuardia AngelaGuardia self-assigned this Jan 31, 2021
@AngelaGuardia AngelaGuardia mentioned this pull request Jan 31, 2021
13 tasks
@h-m-m
Copy link
Collaborator

h-m-m commented Feb 1, 2021

@AngelaGuardia I like a nice clean solution like this

Since this change affects the UI, can you post "before" and "after" screenshots?
Also, do you mind adding a test for this? I'm happy to help with that if our testing setup isn't one you're familiar with

@AngelaGuardia
Copy link
Contributor Author

@AngelaGuardia I like a nice clean solution like this

Since this change affects the UI, can you post "before" and "after" screenshots?
Also, do you mind adding a test for this? I'm happy to help with that if our testing setup isn't one you're familiar with

@h-m-m I added the screenshots! I'm not sure how the map decides which listings to display (it usually just displays one). In this case, the one it displayed on main was different than the one on my branch but you can still see how the name is not shown anymore.

I would love to pair on adding tests! Both to this PR and the other I'm working on. I'm unfamiliar with the setup you're currently using.

@h-m-m
Copy link
Collaborator

h-m-m commented Feb 3, 2021

Great! The screenshots look good

I'm happy to go over our testing setup with you tomorrow during the project's usual Zoom call

@AngelaGuardia AngelaGuardia force-pushed the remove_name_in_map_view branch from 70a75df to 9075df4 Compare February 12, 2021 01:07
@AngelaGuardia AngelaGuardia force-pushed the remove_name_in_map_view branch from 9075df4 to c9287e5 Compare February 12, 2021 01:18
Copy link
Collaborator

@solebared solebared left a comment

Choose a reason for hiding this comment

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

Sweet! 🍠 🙌🏾

@solebared solebared merged commit d230abf into rubyforgood:main Feb 13, 2021
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.

3 participants