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

Allow loadout naming #219

Closed
wants to merge 16 commits into from
Closed

Allow loadout naming #219

wants to merge 16 commits into from

Conversation

NickKoester
Copy link
Contributor

@NickKoester NickKoester commented Feb 2, 2024

Allows users to change the name of their loadouts . When a loadout is created from a preset it will use that name as the default instead of "Loadout n".

image
image
image

closes #146
Also could be an enabler for #217

@LlemonDuck
Copy link
Collaborator

Thanks for the PR. We've been having discussions moreso on whether we should implement this, rather than how, but it's nice to have this work done for us if we ultimately accept it 😄

We'll get back to you with a response soon.

Copy link
Member

@jayktaylor jayktaylor 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 the PR.

I think we're willing to support this, given that the UI you have created is reasonably minimal.

I've committed an adjustment to your code which removes the tick and cross buttons, and instead has a simple flow where clicking on the pencil (which I've made slightly smaller, to be less jarring) focuses your input on the text box, and then you can type a name and hit Enter or click away from the element to save. I think this is a simpler and easier to understand UX.

Additional feedback:

  • When you load an existing shortlink (AKA one created before this PR) with multiple loadouts in it, each loadout except the first has an empty string for loadout.name. This should be changed (probably in state.tsx where we import the data) so that loadouts with a missing name key are named after the array index.
  • Do we want to set a reasonable character limit for loadout names? My feeling is yes, maybe 24 characters max.
  • Do we want to show the loadout name in the results table? I think we do, but how are we going to make it look good with a longer name? Do we wrap the line like below, with a small line-height? Do we truncate (probably not, imo)?
Screenshot 2024-02-02 at 18 35 25

src/state.tsx Outdated Show resolved Hide resolved
@NickKoester
Copy link
Contributor Author

  • Set default name if using old shared loadouts
  • Set max input length of 24 characters
  • Added loadout names to results table

src/state.tsx Outdated Show resolved Hide resolved
src/state.tsx Outdated Show resolved Hide resolved
src/state.tsx Show resolved Hide resolved
src/types/State.ts Outdated Show resolved Hide resolved
src/app/components/player/equipment/EquipmentPresets.tsx Outdated Show resolved Hide resolved
@jayktaylor jayktaylor enabled auto-merge (rebase) February 3, 2024 05:22
@jayktaylor
Copy link
Member

We moved some stuff around with branches. The base branch for the repo (and PRs) is now staging. We'd already merged this PR into that branch, but GitHub is confused by the history so won't let me change the base branch of this PR.

I'm going to close this PR, but your changes were merged in d652498. It will go out to the production site in our next deployment (along with the reverse DPS, etc changes in the staging branch). You can test here.

@jayktaylor jayktaylor closed this Feb 5, 2024
auto-merge was automatically disabled February 5, 2024 00:40

Pull request was closed

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.

Custom named loadouts
3 participants