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

Criminal Records Computer Better UX + Filtering #32352

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

jamessimo
Copy link
Contributor

About the PR

Updated the criminal records computer to have some new features, these are;

  • Show crews jobs and job icon
  • Show criminal status icon
  • Allow for multiline wanted and suspect reasons
  • Ability to filter the criminal record types
  • Fixed a bug where "Suspect" and "Wanted" where used incorrectly.
  • Tweaks to the UI

Why / Balance

When you are joining mid round, there is no way to know who is wanted/detained and on parole unless you manually click through the criminal records. This allows new officers to quickly catch up on whos wanted and for what. Also nice to see jobs because a name would be wanted but no indication of what there job is.

Technical details

Added a new CriminalRecordSetStatusFilter method, this is a int of 0 to 3 which is mapped to CriminalRecord.Status. Also removed some hard coded colors in the locals file.

Orginally this used tabs but the tabs were not the correct method since I didn't want a completely new UI per tab and just needed to filter the stations records according to their criminal record status.

I accidently included a .vscode config file, didn't notice until now and not sure if that will cause an issue.

Media

image

Requirements

Breaking changes

Changelog

🆑

  • tweak: Criminal Records computer got enhanced UX & UI
  • tweak: Criminal Records computer can now filter its list against crew criminal status

@github-actions github-actions bot added Changes: UI Can be reviewed or fixed by people who are knowledgeable with UI design Status: Needs Review This PR requires new reviews before it can be merged. labels Sep 21, 2024
@Moomoobeef
Copy link
Contributor

Moomoobeef commented Sep 21, 2024

you could add a print button and a dropdown to have criminal records be printable to a fax machine, but that might be out of scope for this PR.

@thebadman4662
Copy link

When you are joining mid round, there is no way to know who is wanted/detained and on parole unless you manually click through the criminal records.

Small correction here, we now have wanted lists in PDA showing only criminals, their age, species and job on top of their status. But of course even better criminal records never hurt.

@jamessimo
Copy link
Contributor Author

you could add a print button and a dropdown to have criminal records be printable to a fax machine, but that might be out of scope for this PR.

That was on my plate to do and I even designed it but getting the filtering to work took way too much time. I will make another PR later with that and embed the crime history onto the main console window as outlined in #24646

Copy link
Contributor

@ArchRBX ArchRBX left a comment

Choose a reason for hiding this comment

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

Code looks good to my eyes, just some unclear comments and unnecessary files included.

.vscode/launch.json Outdated Show resolved Hide resolved
@jamessimo
Copy link
Contributor Author

Pretty sure I fixed all the issues. Please let me know if anything needs to change.

@slarticodefast slarticodefast added the Undergoing Maintainer Discussion This PR is currently going through an internal discussion by the maintainer team. label Oct 3, 2024
Loc NA and use inject deps
Cannot use inject deps on sprite system.
@jamessimo
Copy link
Contributor Author

Just wanted to poke and say this PR has no outstanding issues.

@superjj18
Copy link
Contributor

superjj18 commented Oct 15, 2024

Is there a way to search for people of a certain job?

if someone screams “a chef just freakin STABBED ME” it would make it easier to narrow down the suspects.

would very much help AI who doesn’t have access to the manifests

@jamessimo
Copy link
Contributor Author

Is there a way to search for people of a certain job?

if someone screams “a chef just freakin STABBED ME” it would make it easier to narrow down the suspects.

would very much help AI who doesn’t have access to the manifests

I could. I would add that into the next version which will have printing pages and showing crime history at all times. don't want to add more to this PR still hasn't been pulled for a month so adding more will only delay it.

@jamessimo
Copy link
Contributor Author

Just another bump to say this PR is good to go in my eyes.

@chromiumboy chromiumboy self-assigned this Nov 11, 2024
@chromiumboy
Copy link
Contributor

Thanks for this, I'll review it a bit later

Copy link
Contributor

@chromiumboy chromiumboy left a comment

Choose a reason for hiding this comment

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

I really like the changes to the UI you've made, it's a big improvement.

@@ -52,4 +52,4 @@
"preLaunchTask": "build"
}
]
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Try re-adding the white space to remove the changes to this file. If the line endings have been changed, try this: https://ohshitgit.com/#undo-a-file

@@ -31,6 +31,13 @@ public sealed partial class CriminalRecordsConsoleComponent : Component
[DataField]
public StationRecordsFilter? Filter;

/// <summary>
/// Current tab of the filter by criminal status buttons.
///
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
///

@@ -15,6 +15,8 @@

- The criminal records themselves

- Filter buttons below the crew list to show only the wanted, detained or paroled crew.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
- Filter buttons below the crew list to show only the wanted, detained or paroled crew.
- The filter button below the crew list can be used to show only wanted, detained, or paroled crew.

Comment on lines +203 to +208
var statusMap = new Dictionary<int, SecurityStatus>
{
{ 1, SecurityStatus.Wanted },
{ 2, SecurityStatus.Paroled },
{ 3, SecurityStatus.Detained }
};
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be defined outside a function

@@ -28,7 +29,6 @@ public sealed class CriminalRecordsConsoleSystem : SharedCriminalRecordsConsoleS
[Dependency] private readonly StationRecordsSystem _records = default!;
[Dependency] private readonly StationSystem _station = default!;
[Dependency] private readonly UserInterfaceSystem _ui = default!;

Copy link
Contributor

Choose a reason for hiding this comment

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

Unnecessary whitespace change

@@ -162,7 +211,7 @@ public void UpdateState(CriminalRecordsConsoleState state)
}
}

private void PopulateRecordListing(Dictionary<uint, string>? listing)
private void PopulateRecordListing(Dictionary<uint, string>? listing, SecurityStatus? filterSecStatus = null)
Copy link
Contributor

Choose a reason for hiding this comment

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

filterSecStatus appears to be unused

FontColorOverride="DarkGray" />
<!-- Selected record info -->
<BoxContainer Name="PersonContainer"
Orientation="Vertical"
Copy link
Contributor

Choose a reason for hiding this comment

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

This container should expand when the window is resized

<BoxContainer Margin="5 5 5 10"
HorizontalExpand="true"
VerticalAlignment="Center">
<OptionButton Name="FilterType"
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider increasing the width of the filter button so it matches the width of the crew list below it

Margin="5 5 10 5"
MinWidth="250"
MaxWidth="250">
<Label Name="RecordListingTitle"
Copy link
Contributor

Choose a reason for hiding this comment

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

A little extra whitespace to pad the title from the list below would be nice, plus maybe increase the font size a little

<RichTextLabel Name="WantedReason" Visible="False"/>
<Button Name="HistoryButton" Text="{Loc 'criminal-records-console-crime-history'}"/>
</BoxContainer>
<BoxContainer Orientation="Horizontal"
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd like to see this container shrunk down so it matches the width of the crew list above it. Consider using a drop-down list for the buttons (when these labels are translated into Russian, for example, they are quite long)

@chromiumboy chromiumboy added Status: Awaiting Changes This PR needs its reviews addressed or changes to be made in order to be merged. and removed Status: Needs Review This PR requires new reviews before it can be merged. Undergoing Maintainer Discussion This PR is currently going through an internal discussion by the maintainer team. labels Nov 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Changes: UI Can be reviewed or fixed by people who are knowledgeable with UI design Status: Awaiting Changes This PR needs its reviews addressed or changes to be made in order to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants