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

WIP : Update Navigation #1036

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

AllanOcelot
Copy link
Contributor

@AllanOcelot AllanOcelot commented Jan 21, 2025

Navigation Rework

  • Various styling tweaks and improvements to give the navigation a more polished and consistent feel. This is an hour or two of work, so I'm not precious about anything - personally I think it's a big improvement, but it needs some more polish and the code needs cleaning up before we consider merging.

Description 🗒️

  • Reordered navigation
  • Added new item - "Tools" for all the calc/tools to sit under
  • Re styled elements ( spacing, padding, background etc )
  • Moved settings and remote icon to right side
  • Hover Effect for menu item ( activation ) more consistent
  • Added mobile button to toggle mobile menu
  • Styled menu on mobile
  • Removed the 'automatic' hiding mechanism, that was hiding items if they did not fit on menu

Builds locally.

Examples 📸

Desktop

image

Mobile Menu Closed

image

Mobile Menu Open

image

@AllanOcelot AllanOcelot requested a review from a team as a code owner January 21, 2025 19:14
@Razzmatazzz
Copy link
Member

.deploy to development

Copy link
Contributor

⚠️ Cannot proceed with deployment

  • reviewDecision: REVIEW_REQUIRED

All deployments from forks must have the required reviews before they can proceed. Please ensure this PR has been reviewed and approved before trying again.

@Razzmatazzz
Copy link
Member

.deploy to development

Copy link
Contributor

Deployment Triggered 🚀

Razzmatazzz, started a branch deployment to development (branch: 19b7ee855d36b8ae8106d74e21907753087e8806)

You can watch the progress here 🔗

Details
{
  "type": "branch",
  "environment": {
    "name": "development",
    "url": null
  },
  "deployment": {
    "timestamp": "2025-01-22T22:44:13.725Z",
    "logs": "https://github.com/the-hideout/tarkov-dev/actions/runs/12918455745"
  },
  "git": {
    "branch": "19b7ee855d36b8ae8106d74e21907753087e8806",
    "commit": "19b7ee855d36b8ae8106d74e21907753087e8806",
    "verified": false
  },
  "context": {
    "actor": "Razzmatazzz",
    "noop": false,
    "fork": true,
    "comment": {
      "created_at": "2025-01-22T22:43:51Z",
      "updated_at": "2025-01-22T22:43:51Z",
      "body": ".deploy to development",
      "html_url": "https://github.com/the-hideout/tarkov-dev/pull/1036#issuecomment-2608419557"
    }
  },
  "parameters": {
    "raw": null,
    "parsed": null
  }
}

@github-actions github-actions bot deployed to development January 22, 2025 22:44 Active
Copy link
Contributor

Deployment Results ✅

Razzmatazzz successfully deployed branch 19b7ee855d36b8ae8106d74e21907753087e8806 to development

Show Results

https://51e37d57.tarkov-dev.pages.dev

Details
{
  "status": "success",
  "environment": {
    "name": "development",
    "url": null
  },
  "deployment": {
    "id": 2134906202,
    "timestamp": "2025-01-22T22:46:06.405Z",
    "logs": "https://github.com/the-hideout/tarkov-dev/actions/runs/12918455745",
    "duration": 113
  },
  "git": {
    "branch": "19b7ee855d36b8ae8106d74e21907753087e8806",
    "commit": "19b7ee855d36b8ae8106d74e21907753087e8806",
    "verified": false
  },
  "context": {
    "actor": "Razzmatazzz",
    "noop": false,
    "fork": true
  },
  "reviews": {
    "count": 1,
    "decision": "APPROVED"
  },
  "parameters": {
    "raw": null,
    "parsed": null
  }
}

Copy link
Member

@Razzmatazzz Razzmatazzz left a comment

Choose a reason for hiding this comment

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

Deployed here for previewing: https://preview.tarkov-dev.pages.dev/settings/

One thing I notice right away is that it's not workable to have the menu collapse according to specific pixel widths because we can't be sure how much width each menu item will take up due to localization. For example, if you switch the site language to ru, some window widths drive the remote and settings buttons off the screen. This is a large part of the reason behind the current "dynamic" system of shunting menu items to the overflow menu once they become too long: it works with all localizations, regardless of how many characters each menu item ends up being.

@AllanOcelot
Copy link
Contributor Author

Deployed here for previewing: https://preview.tarkov-dev.pages.dev/settings/

Good point,
I'll kick about some ideas. I think there's a good ground somewhere - for instance putting Achievements into the "other" menu. Or placing "Loot Teirs" into the tools section etc.

As discussed we don't really want to disrupt the menu too much, and I'm not in a rush to get this stuff merged in, so we have some good time for community feedback

@Razzmatazzz
Copy link
Member

For sure. There are some good improvements here, and it will likely fix #791 as well. The trick is finding a good balance that doesn't stick often-used pages deep into nested menus and also works with the varying element widths inherent with all the different translations.

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