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

Revamp numeric text field #550

Merged
merged 8 commits into from
Nov 5, 2024
Merged

Conversation

glmdgrielson
Copy link
Contributor

This SHOULD fix bug #414, in a way that doesn't look too bad.

βœ… Changes

  • refactor: change the text display used for numeric text inputs.

πŸŒ„ Context

Per bug #414, the prior version had a limit on how large the value in the field could be.

πŸ”’Checklist

  • I tested my work on the feature environment

This SHOULD fix bug farirpgs#414, in a way that doesn't look too bad.
@RPDeshaies
Copy link
Collaborator

Just tested, the UI looks fine!

Though we'd need to disable the logic to prevent the 999 cap here: https://github.com/glmdgrielson/fari-app/blob/9900754feb3c207fc04dd695825810e3f64134d8/lib/routes/Character/components/CharacterDialog/components/CircleTextField.tsx#L95-L109

and probably change this forced with of 3rem here: https://github.com/glmdgrielson/fari-app/blob/9900754feb3c207fc04dd695825810e3f64134d8/lib/routes/Character/components/CharacterDialog/components/CircleTextField.tsx#L113-L114

@glmdgrielson
Copy link
Contributor Author

One other thing that might need fixing, this warning:

transforming (1498) node_modules/@mui/x-data-grid/locales/bgBG.js[plugin:vite:esbuild] Duplicate key "background" in object literal
117|                "outline": "none",
118|                "background": props.highlight ? miniTheme.textPrimary : "inherit",
119|                "background": theme.palette.action.hover,
   |                ^
120|                "&&": {
121|                  color: "inherit",

The whole point was to stop doing that, after all.
Now to hope that this actually looks decent.
@glmdgrielson
Copy link
Contributor Author

Alright, both of the issues you brought up have been fixed. (I can't believe I forgot to remove the code that causes literally the very issue I was supposed to be solving the whole time.)

I could have sworn that read "background-color". Apparently it didn't.
WHOOPS! Well, that's fixed now.
@RPDeshaies RPDeshaies enabled auto-merge (squash) November 5, 2024 18:29
@RPDeshaies RPDeshaies disabled auto-merge November 5, 2024 18:29
@RPDeshaies RPDeshaies merged commit 9dc2d3c into farirpgs:master Nov 5, 2024
3 of 4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants