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 /contributions link in navbar when p2p enabled #821

Merged
merged 1 commit into from
Jan 18, 2021

Conversation

h-m-m
Copy link
Collaborator

@h-m-m h-m-m commented Jan 15, 2021

Why

We want to show the /contributions link in navbar when p2p enabled because it's a key part of the p2p workflow

Pre-Merge Checklist

  • All new features have been described in the pull request
  • Security & accessibility have been considered
  • 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
  • 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

What

Screenshots of current code:

Logged out with P2P
Screen Shot 2021-01-14 at 7 48 16 PM

Logged out without P2P
Screen Shot 2021-01-14 at 7 45 02 PM

Logged in
Screen Shot 2021-01-14 at 7 40 09 PM

How

We're adding a prop passed to Vue that indicates if P2P is on or not, and then letting Vue show or hide the button

Testing

There are some JS tests since this is mostly a JS change

Next Steps and Concerns

We talked some about rearranging these. Our concerns were:

  • arranging buttons so items of higher priority or higher usefulness stand out more, which changes depending on context
  • if someone logged in trying is to give instruction to someone not logged, it should be easy without the logged in person having to memorize which buttons are and are not the same for the logged out person
  • having order be consistent and predictable
  • understanding that most logged in users won't be spending much time if any looking at what a not logged in person sees

Because of all these concerns, we agreed to revisit how to balance them later

Accessibility

We did not do accessibility testing for this. It seems unlikely to me that it would change the accessibility from the previous code

Security

This may reveal the link URL in the JS to someone who is not logged in. That said, that URL will still have the Devise checks, so the additional insecurity is only removing obscurity.

Co-authored-by: lasitha <exbinary@gmail.com>
Co-authored-by: maebeale <maebeale@gmail.com>
Co-authored-by: svileshina <svileshina@gmail.com>
Co-authored-by: h-m-m <h-m-m@users.noreply.github.com>
@h-m-m h-m-m force-pushed the p2p/show-contributions-link branch from 6473c25 to 6795912 Compare January 15, 2021 01:06
@solebared
Copy link
Collaborator

Mob reviewed!

@solebared solebared merged commit fb310b4 into main Jan 18, 2021
@solebared solebared deleted the p2p/show-contributions-link branch January 18, 2021 22:56
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.

2 participants