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

Changed "Manage Autolab" button from a page redirect to a dropdown #1758

Merged
merged 22 commits into from
Feb 12, 2023

Conversation

KesterTan
Copy link
Contributor

Description

  • Deleted show page which included 5 separate links.
  • Changed "Manage Autolab" from a redirect button into a dropdown list

Screenshot of the new dropdown list on desktop

Screenshot 2023-02-01 at 11 17 49 PM

Screenshot of the new dropdown list on mobile

Screenshot 2023-02-02 at 11 38 26 AM

Motivation and Context

Originally, clicking the button redirects the user to this page with five links.

Screenshot 2023-02-01 at 11 33 18 PM

There's no value in having those links in a separate page since they are only five links. Hence, the dropdown.

How Has This Been Tested?

Enter developer login below the login page.
Screenshot 2023-02-02 at 11 46 29 AM

Click the Manage Autolab button on the top right hand corner
Screenshot 2023-02-02 at 11 46 56 AM

Change the browser width to view the change in the mobile sidebar as well.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • I have run rubocop for style check. If you haven't, run overcommit --install && overcommit --sign to use pre-commit hook for linting
  • My change requires a change to the documentation, which is located at Autolab Docs
  • I have updated the documentation accordingly, included in this PR

@KesterTan KesterTan marked this pull request as ready for review February 2, 2023 16:48
@gitstream-cm gitstream-cm bot requested a review from damianhxy February 2, 2023 16:49
@20wildmanj
Copy link
Contributor

20wildmanj commented Feb 3, 2023

Can you update your PR so that it removes any RSpec tests for admins/show?

@damianhxy
Copy link
Member

You should fully remove any code related to the admin#show method.

In config/routes.rb:
Replace L70 with: resource :admin, :except => [:show] do

In controllers/admins_controller.rb:
Remove L11-12:

  action_auth_level :show, :administrator
  def show; end

@damianhxy
Copy link
Member

damianhxy commented Feb 5, 2023

Currently, the "Manage Autolab" text flashes white every time you click on it (or clicking any other button, really).

Screenshot 2023-02-04 at 22 30 47

The offending piece of code seems to be navbar.css L12-15:

nav a, nav a:hover, nav a:focus{
  text-decoration: none;
  color:white;
}

Which is unnecessary because the normal navbar items already have another style applied
Screenshot 2023-02-04 at 22 29 40

RECOMMENDATION: Delete the code

@damianhxy
Copy link
Member

Right now, if a user is signed out, the sidebar nav shows no options at all:
Screenshot 2023-02-04 at 22 28 23

The problem is that the highlighted piece of code is within the <% if user_signed_in? %> ... <% end %> block when it shouldn't be
Screenshot 2023-02-04 at 22 32 19

RECOMMENDATION: Move the highlighted chunk of code outside of the block.

@damianhxy
Copy link
Member

Currently, clicking on the dropdown simply triggers it open again, rather than closes it. Is there a way to change this behavior?

Copy link
Member

@damianhxy damianhxy left a comment

Choose a reason for hiding this comment

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

Left some comments

app/views/layouts/_navbar.html.erb Outdated Show resolved Hide resolved
app/views/layouts/_navbar.html.erb Outdated Show resolved Hide resolved
app/views/layouts/_navbar.html.erb Show resolved Hide resolved
@gitstream-cm gitstream-cm bot requested a review from victorhuangwq February 5, 2023 03:38
@gitstream-cm gitstream-cm bot requested a review from 20wildmanj February 5, 2023 19:14
@gitstream-cm gitstream-cm bot requested a review from najclark February 5, 2023 19:20
Copy link
Member

@damianhxy damianhxy left a comment

Choose a reason for hiding this comment

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

Thanks for resolving some comments, but some were unaddressed and some new issues were introduced

db/schema.rb Outdated Show resolved Hide resolved
Gemfile Outdated Show resolved Hide resolved
.gitignore Outdated Show resolved Hide resolved
app/views/assessments/index.html.erb Outdated Show resolved Hide resolved
app/views/layouts/_navbar.html.erb Outdated Show resolved Hide resolved
app/views/layouts/_navbar.html.erb Outdated Show resolved Hide resolved
app/views/layouts/_navbar.html.erb Show resolved Hide resolved
app/views/layouts/_navbar.html.erb Show resolved Hide resolved
Copy link
Member

@damianhxy damianhxy left a comment

Choose a reason for hiding this comment

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

  • Left a nit about spacing
  • Iffy dropdown behavior persists, but it is not a big deal and can be resolved in a future PR if necessary (also, only happens when I enable touch simulation on firefox, not sure if it actually affects mobile)

lgtm

@KesterTan KesterTan added this pull request to the merge queue Feb 12, 2023
@damianhxy damianhxy removed this pull request from the merge queue due to a manual request Feb 12, 2023
@damianhxy damianhxy added this pull request to the merge queue Feb 12, 2023
Merged via the queue into master with commit 658cd94 Feb 12, 2023
@damianhxy damianhxy deleted the kester branch February 12, 2023 18:43
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