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

Issue #1000 Responsive design changes and element overhaul to Player Loadout in tarkov.dev/players/* pages #1002

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

Conversation

Razzmatazzz
Copy link
Member

@Razzmatazzz Razzmatazzz commented Oct 28, 2024

Changes courtesy of @Applefrittr

Responsive Design and UI overhaul to Player loadouts in tarkov.dev/players/* Issue #1000

Targeted changes to provide a better user experience, especially on phone.

Description 🗒️

Refer to Issue #1000. The original loadout element caused page layout concerns, especially on smaller screens. Overflow issues fixed and overall look of the element was changed dramatically to give the look and feel of the in game character screen.

KEY CHANGES

  1. A new data field was added to fetched items from the API - 'inspectImageLink' (file: src/features/items/do-fetch-items.mjs)
  2. A new React hook was added to the hooks dir - 'useResponsiveScaling.jsx'. This hook works in conjunction with components/item-image/index.js to scale up/down images passed from pages/player/index.js based on screen size - RESPONSIVE 😃
  3. CSS media queries added to pages/player/index.css to match screen breakpoints set up in useResponsiveScaling.jsx. Grid layout overhaul.
  4. IMPORTANT!!!!!!!!!! I commented out the CloudFlare turnstile component, import statement, and the call to fetchProfile() is useEffect() in src/pages/player/index.js for ease of development. The CF turnstile prevents anything from happening on the page unless the capcha is passed. Unfortunately a request from localhost doesn't cut it 🫤 These changes NEED to be reversed before merge!!!!!! I left comments tagged with issue#1000 in the file itself to locate the changes easily.
  5. In order to get a profile loaded into my local environment for testing, I saved a few dummy profiles from the live site and used the LoadProfile feature on the tarkov.dev/players/* page. Those json files are located in src/pages/player/saved-profiles-issue#1000. Since there is no way (not that I found) to navigate to a unique tarkov.dev/players/* page, just type it in directly to the address bar, example: tarkov.dev/palyers/regular/1. This will bring up the unique player page with nothing loaded yet. Simply click the Load Profile button and pick a file from the added directory. Delete saved profiles directory before merge!
  6. My linter/Prettier kind of went crazy on the files I worked on, bloating the changes; I'm unsure if that is standard or not. Still learning 😄

Examples 📸

loadout1
responsive1
responsive2
responsive3

Related Issues 🔗

I explored the site after my changes in my local environment and did not notice any changes to other image elements across the site 🤞

First PR, be gentle 😃


Expand for Help

By submitting this pull request, you agree to our code of conduct

Preview: https://preview.tarkov-dev.pages.dev/players

Issues blocking this from merging:

  • the item-image element now supports maxImageWidth and maxImageHeight properties. These are the maximum width and height of the image at large (desktop) resolutions, and they scale down as screen size decreases.

Resolves #1000

@Razzmatazzz Razzmatazzz requested a review from a team as a code owner October 28, 2024 21:14
Copy link
Contributor

👋 Thanks for opening a pull request!

If you are new, please check out the trimmed down summary of our deployment process below:

  1. 👀 Observe the CI jobs and tests to ensure they are passing

  2. ✔️ Obtain an approval/review on this pull request

  3. 🚀 Deploy your pull request to the development environment with .deploy to development

  4. 🚀 Deploy your pull request to the production environment with .deploy

    If anything goes wrong, rollback with .deploy main

  5. 🎉 Merge!

Note: If you have a larger change and want to block deployments, you can run .lock --reason <reason> to lock all other deployments (remove with .unlock)

You can view the branch deploy usage guide for additional information

@Razzmatazzz
Copy link
Member Author

.deploy to development

Copy link
Contributor

Deployment Triggered 🚀

Razzmatazzz, started a branch deployment to development

You can watch the progress here 🔗

Branch: responsive-loadouts

Copy link
Contributor

Deployment Results ✅

Razzmatazzz successfully deployed branch responsive-loadouts to development

Show Results

https://c4093f8e.tarkov-dev.pages.dev

@Applefrittr
Copy link

Applefrittr commented Nov 8, 2024

Hey Razz! Just to address your points above:

  • Changes were made to /components/item-image to arbitrarily restrict the size of the component when using the inspect image, which I don't think is the best approach because it's conceivable that his component could be used in different contexts to display that image at another size

It look like the tweaks commit you pushed takes care of this no? Now you can feed image dimensions directly into the evocation of the item-image component. Pretty nice as gives the component more flexibility without have image dimensions hard coded into the component itself. With this change we can also scale down the attachment images when a user clicks on the weapon if need be (should there be some sort of icon or message that the user CAN click to expand?)

  • I think it would be better to use something that looks more like grid images for the item images, as that's how the loadout screen appears

Specifically which image form the item object would be the best fit? baseImageLink, gridImageLink?

Side suggestion: Are we dead set on using the Mui X package to display the attachment list? What if we did something like this from in game when a user clicks a weapon for example? Just list all attachments in a flex display with wrapping without displaying empty slots (you'd have to know all the different slots available on all weapons across the game, which sounds like a nightmare). Just an idea...

attachments

@Razzmatazzz
Copy link
Member Author

Hey @Applefrittr! Yes, we're close to getting this merged. I added the points above just so you'd have some indication of the current status of things and not feel like your PR disappeared into a black hole. I'm currently experimenting with styling the images more like the grid images that appear in the loadout slots in the game while keeping them responsive:
image

@Razzmatazzz
Copy link
Member Author

.deploy to development

Copy link
Contributor

Deployment Triggered 🚀

Razzmatazzz, started a branch deployment to development

You can watch the progress here 🔗

Branch: responsive-loadouts

Copy link
Contributor

Deployment Results ✅

Razzmatazzz successfully deployed branch responsive-loadouts to development

Show Results

https://96a136ca.tarkov-dev.pages.dev

@Razzmatazzz
Copy link
Member Author

@Applefrittr what do you think of this implementation?
https://preview.tarkov-dev.pages.dev/players

@Applefrittr
Copy link

Applefrittr commented Nov 12, 2024

@Razzmatazzz
Messing around with responsive design mode in firefox dev tools, grabbed this pic:
newloadout

Oh yeah we're making progress. A few nit picks though:

  1. The weapon image for whatever reason is too small, in comparison to every other item slot. I couldn't get it to stretch to fill the grid slot myself when I submitted my PR as well
  2. The item name text are all different sizes, anyway to make them all uniform? I remember there being an item text prop in item-image component (look at the armband name and compare to any other item slot). Maybe apply the same thing to the item durability values?
  3. And this is just a personal thing but the melee weapon drives me a little crazy. Since its a 3x1 grid size, it expands the second weapon slot by another row, making the weapon slots not uniform. Maybe set grid rows max-height or maybe use object-fit: contain on the img elements? (OR this could just be related to my first point above with the primary weapon img being so small...)

BUT, we did fix the initial complaints I made when I submitted the issue. The overall element fits on smaller screens, doesn't distort the rest of the page, and we got slot labels too 😄 The rest of the changes I guess are more subjective

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.

Responsive design/display issues and overall layout of Loadout in tarkov.dev/players/* page
2 participants