-
Notifications
You must be signed in to change notification settings - Fork 46
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
Allow loadout naming #219
Conversation
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. |
There was a problem hiding this 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 missingname
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)?
|
We moved some stuff around with branches. The base branch for the repo (and PRs) is now 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. |
Pull request was closed
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".
closes #146
Also could be an enabler for #217