433-365 : dropdown on navbar and profile #963

vurtn · 2021-05-20T04:19:39Z

Sorry for my english.

Why

The main issue i tried to solve (#433) is about adding a dropdown in the right part of the navbar. I added a link to edit the profile of the current user (#365).

What

Add a dropdown menu in the navbar
Create some items in it
Change the z-index of the navbar
Add a new component DeleteLink
Change the FeedbackButton
Change some things in NavBar.spec.js
Add a test in user_spec.rb

How

I added the dropdown in the navbar with Buefy and had to make some changes.

First was the buttons/button style in the navbar. I added a parameter to FeedbackButton because it is already a link and the value by default is the button style (it's keeping the current style by default to not break some code elsewhere (and it's name is Button so... 😄 ). The button to disconnect the user is a real button so i created another component to have the same behavior but with a link.

I used the Logout condition to show the dropdown in the navbar and added a z-index on the navbar because some components are above sometimes. It's visible on small screens like on this gif :

The burger on the page is above the navbar.

In the dropdown menu there is a link to edit our profile :

Testing

Some test failed due to the new structure of the navbar in NavBar.spec.js. The main issue was this part :

const navLinks = $wrapper.findAll('a.navbar-item').wrappers
const navButtons = $wrapper.findAll('.navbar-item a').wrappers
const navForms = $wrapper.findAll('.navbar-item form').wrappers
return [...navLinks, ...navButtons, ...navForms].map(link => link.text())

The dropdown div is a navbar-item, so the links inside it match a.navbar-item and .navbar-item a selectors. The return is an array and not a set so i had duplicates and the test failed. I changed the lines above to :

const navLinks = $wrapper.findAll('a:not(.burger)').wrappers
return [...navLinks].map((link) => link.text())

I kept the spread operator because if we need to select something else, we will just add the new catches to this array like before. The navbar contains only links now so the only duplicate i had was the root page because of the burger link for the small screens. I excluded it of the query selector. I added the new links to the test of admin buttons.

I added a test in user_spec.rb to verify that the user is redirected to the sign in page if he is not logged in while trying to access the profile page.

Next Steps

Unknown. Maybe create some tests with a context like "with the right credentials" (and its complementary of course) to check the access to resources.

Outstanding Questions, Concerns and Other Notes

Webpack throwing this error sometimes when i'm testing and i still don't understand why :

ActionView::Template::Error:
       Webpacker can't find application.css in /my-path/mutual-aid/public/packs-test/manifest.json.

Accessibility

Not seem to be concerned now.

Security

Not seem to be concerned (now ?).

Pre-Merge Checklist

I think the reviewer will check this so i let it empty.

Security & accessibility have been considered
Tests have been added, or an explanation has been given why the features cannot be tested
Documentation and comments have been added to the codebase where required
Entry added to CHANGELOG.md if appropriate
Outstanding questions and concerns have been resolved
Any next steps have been turned into Issues or Discussions as appropriate

Best regards and stay safe !

solebared · 2021-10-20T16:58:54Z

app/javascript/components/forms/DeleteLink.vue

+  computed: {
+    authenticityToken() {
+      return document.querySelector('meta[name="csrf-token"]').content
+    },
+  },


I don't think this authenticityToken is required. I see the existing DeleteButton has it as well but i think that was left there by mistake and is not used there either.

solebared

Thanks so much for working on this @vurtn 🙏🏾 , the changes look great! 🚀 .
Apologies for the long delay in reviewing this.
A few minor comments and questions below. No worries if you don't get to them, i'm happy to merge this in and tackle any further changes ourselves.

solebared · 2021-10-20T17:11:19Z

app/javascript/components/forms/index.js

-import SubmitButton     from './SubmitButton'
+import SpacerField from './SpacerField'
+import SubmitButton from './SubmitButton'
+import DeleteLink from './DeleteLink'


I'm guessing you have prettier or some other auto-formatting tool enabled. Not a big deal in this case, but for future changes, would be great if you could disable it or have it respect our project settings (based on StandardRB with some customizations.

solebared · 2021-10-20T17:18:37Z

spec/requests/users_spec.rb

+      expect(response).not_to be_successful
+      expect(response.response_code).to eq(302) # Redirection
+      expect(response.header["Location"]).to eq('http://www.example.com/users/sign_in')


Thanks for adding this spec! I think the expectations above can be simplified to:

Suggested change

expect(response).not_to be_successful

expect(response.response_code).to eq(302) # Redirection

expect(response.header["Location"]).to eq('http://www.example.com/users/sign_in')

expect(response).to redirect_to new_user_session_path

solebared · 2021-10-20T18:02:07Z

app/assets/stylesheets/application.scss

+
+.navbar {
+  z-index: 10;
+}


I wasn't able to recreate the problem you mentioned with the 'burger' showing up in the wrong place. When i comment out this z-index, everything still seems to work fine. Could you double check and describe steps to recreate if you're still seeing it happen?

solebared · 2021-10-20T18:03:39Z

app/javascript/components/NavBar.vue

+        <div class="sub-item">
+          <FeedbackButton
+            classes="navbar-item"
+            action="software_feedbacks/new"


This action property seems to be obsolete (i know it existed before your changes -- must've been inadvertently left behind previously).

solebared · 2021-10-20T18:11:20Z

app/javascript/components/forms/DeleteLink.vue

+  <form :action="action" method="post" ref="form">
+    <AuthTokenInput />
+    <input type="hidden" name="_method" value="delete" />
+    <a href="#" :class="classes" v-on:click="submit"><slot /></a>


Minor: i have a slight preference to put <slot /> on its own line, to give it a little more visibility.
When scanning a component template, it's helpful to be able to quickly spot any slots defined.

solebared · 2021-10-20T18:19:11Z

Webpack throwing this error sometimes when i'm testing and i still don't understand why :
ActionView::Template::Error:
       Webpacker can't find application.css in /my-path/mutual-aid/public/packs-test/manifest.json.   

Yes, i run into this sometimes too and don't fully understand it.
I think it has to do with switching between dev and test, but i need to pin down exact steps to recreate.
Thanks for noting this.

solebared · 2021-10-20T18:34:04Z

I didn't search why but i created a new account in the app and have the volonteer profile. The FeedbackButton is shown in the navbar but when i click on it i have this message :

Sorry, you aren't authorized to do that.
If you think this is wrong, please contact your administrator.

I think something is wrong 😄 The FeedbackButton have the v-if="visible('Feedback')" condition and the item is shown but i am not authorized to access it.

Great find @vurtn! I created #992 to track this.

Also, i appreciate the level of detail in your PR description, including the screenshots and explanations of some of the decisions you made and things you ran into. Very helpful! 🔥

See comments under: rubyforgood#963 (review)

solebared

@vurtn , i went ahead and made the minor changes from the previous review.
Thanks so much for your work on this! 🙌🏾 .

gamecat2d added 2 commits May 20, 2021 04:33

433-365 : dropdown and profile

82858fa

change the test

ee5213b

solebared mentioned this pull request Oct 15, 2021

Tie up loose ends #991

Closed

26 tasks

solebared reviewed Oct 20, 2021

View reviewed changes

solebared mentioned this pull request Oct 20, 2021

Newly registered user is presented a Feedback option but not authorized to access it #992

Closed

solebared added 3 commits December 9, 2021 19:32

Merge branch 'main' into front-on-navbar

4635254

Remove unused authenticityToken from DeleteLink and ...Button

10d2abf

Minor changes on rubyforgood#963 based on review

5501b69

See comments under: rubyforgood#963 (review)

solebared approved these changes Dec 10, 2021

View reviewed changes

solebared merged commit ef0ded1 into rubyforgood:main Dec 10, 2021

solebared mentioned this pull request Dec 20, 2021

Make admin section of Navbar.vue be a dropdown #433

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

433-365 : dropdown on navbar and profile #963

433-365 : dropdown on navbar and profile #963

vurtn commented May 20, 2021

solebared Oct 20, 2021

solebared left a comment

solebared Oct 20, 2021

solebared Oct 20, 2021

solebared Oct 20, 2021

solebared Oct 20, 2021

solebared Oct 20, 2021

solebared commented Oct 20, 2021

solebared commented Oct 20, 2021

solebared left a comment

433-365 : dropdown on navbar and profile #963

433-365 : dropdown on navbar and profile #963

Conversation

vurtn commented May 20, 2021

Why

What

How

Testing

Next Steps

Outstanding Questions, Concerns and Other Notes

Accessibility

Security

Meta

Pre-Merge Checklist

solebared Oct 20, 2021

Choose a reason for hiding this comment

solebared left a comment

Choose a reason for hiding this comment

solebared Oct 20, 2021

Choose a reason for hiding this comment

solebared Oct 20, 2021

Choose a reason for hiding this comment

solebared Oct 20, 2021

Choose a reason for hiding this comment

solebared Oct 20, 2021

Choose a reason for hiding this comment

solebared Oct 20, 2021

Choose a reason for hiding this comment

solebared commented Oct 20, 2021

solebared commented Oct 20, 2021

solebared left a comment

Choose a reason for hiding this comment