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

votelink (pda voting !!!) #481

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

Conversation

MilonPL
Copy link
Contributor

@MilonPL MilonPL commented Nov 22, 2024

About the PR

adds the VoteLink cartridge, a new app that lets you create polls and vote in them!
comes preinstalled on all PDAs
creating polls is restricted to command
5 minute cooldown, voting is predicted, timer updates separately, everything is in the code :godo:

Why / Balance

i thought it'd be cool
with each PR we're closer to PDA messaging

Media

Click me!
hiiiii

image

image

image

image

image

@dffdff2423 dffdff2423 self-requested a review November 24, 2024 05:02
@github-actions github-actions bot added the Changes: UI Changes to UI files. label Nov 26, 2024
Copy link
Collaborator

@dffdff2423 dffdff2423 left a comment

Choose a reason for hiding this comment

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

Looks mostly fine, mainly just stylistic changes below.

I found a few small issues while testing in-game:

  • There is no way for players to see the cooldown. Could you put it in the tooltip?
  • I am fairly certain that the "new vote" button stays disabled after the cooldown elapses requiring you to re-open the UI.
  • There is no cap on time remaining which means that people are able to disable votes for a round by creating a really long vote.
  • Given that it is intended to be a PDA cartridge, it may be better stylistically to not have popups. Instead change the contents of the PDA. However, if this is hard to do it is not a big deal.

Comment on lines +45 to +61
var container = new BoxContainer
{
Orientation = BoxContainer.LayoutOrientation.Horizontal,
Margin = new Thickness(0, 2),
};

var inputPanel = new PanelContainer
{
StyleClasses = { "ButtonSquare" },
HorizontalExpand = true,
};

var input = new LineEdit
{
PlaceHolder = Loc.GetString("vote-link-option-placeholder", ("number", _optionInputs.Count + 1)),
HorizontalExpand = true,
};
Copy link
Collaborator

Choose a reason for hiding this comment

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

Was this a deliberate choice to not create another XAML file for this?

Comment on lines +39 to +45
foreach (var vote in history.OrderByDescending(v => v.StartTime))
{
var entry = new PanelContainer
{
HorizontalExpand = true,
StyleClasses = { "AngleRect" },
Margin = new Thickness(0, 0, 0, 8),
Copy link
Collaborator

Choose a reason for hiding this comment

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

The UI stuff created in this loop probably should be their own XAML file.

Comment on lines +131 to +137
CreateVoteButton.ToolTip = !state.HasAccess
? Loc.GetString("vote-link-no-access")
: _activeVote != null
? Loc.GetString("vote-link-create-disabled")
: onCooldown
? Loc.GetString("vote-link-on-cooldown")
: null; // amazing
Copy link
Collaborator

Choose a reason for hiding this comment

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

This nested ternary operator is incredibly cursed but I will not die on this hill

if (totalVotes > 0)
{
// Display options sorted by vote count
foreach (var option in _lastCompletedVote.Options
Copy link
Collaborator

Choose a reason for hiding this comment

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

Again, might be preferable to create another control for this. However, I am not fully convinced on this one because it is almost entirely created programatically.

return GetPredictedVoteCount(VoteOption.Abstain, vote);
}

private void CreateVoteDisplay(VoteData vote)
Copy link
Collaborator

Choose a reason for hiding this comment

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

This probably should be XAML.

public readonly int Votes = votes;
}

[DataDefinition] [Serializable] [NetSerializable]
Copy link
Collaborator

Choose a reason for hiding this comment

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

above


namespace Content.Shared._CD.CartridgeLoader.Cartridges;

[Serializable] [NetSerializable]
Copy link
Collaborator

Choose a reason for hiding this comment

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

above.

public readonly VoteData? VoteData = voteData;
}

[Serializable] [NetSerializable]
Copy link
Collaborator

Choose a reason for hiding this comment

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

above

RequestHistory,
}

[Serializable] [NetSerializable]
Copy link
Collaborator

Choose a reason for hiding this comment

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

above.

/// <summary>
/// Handles the global VoteLink elements.
/// </summary>
[RegisterComponent] [Access(typeof(VoteLinkSystem))] [AutoGenerateComponentPause]
Copy link
Collaborator

Choose a reason for hiding this comment

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

above.

@MilonPL
Copy link
Contributor Author

MilonPL commented Nov 27, 2024

Given that it is intended to be a PDA cartridge, it may be better stylistically to not have popups. Instead change the contents of the PDA.

yes I hear that but I personally prefer doing popups for stuff like this mainly for accessibility reasons: PDAs have a locked size, but you can resize popups, open multiple at once
it's convenient for taking screenshots, takes less screen space than the entire PDA, etc etc

Copy link
Contributor Author

@MilonPL MilonPL left a comment

Choose a reason for hiding this comment

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

what was i cooking

Comment on lines +3 to +16
public sealed class VoteStartedEvent(EntityUid station) : EntityEventArgs
{
public EntityUid Station = station;
}

public sealed class VoteEndedEvent(EntityUid station) : EntityEventArgs
{
public EntityUid Station = station;
}

public sealed class VoteUpdatedEvent(EntityUid station) : EntityEventArgs
{
public EntityUid Station = station;
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

why are these classes

VoteData? activeVote,
List<VoteData> history,
TimeSpan? cooldown = null,
bool hasAccess = false)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

why just check access on the client

Copy link
Collaborator

Choose a reason for hiding this comment

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

how the fuck did I miss this :(

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i already forgot how this code works, saw this and went "what why the fuck"

Copy link
Collaborator

Choose a reason for hiding this comment

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

That is what you get for coding while sleep-deprived. If I remember correctly I also reviewed this at midnight.

Comment on lines +63 to +76
private void OnVoteStarted(VoteStartedEvent ev)
{
UpdateAllCartridges(ev.Station);
}

private void OnVoteEnded(VoteEndedEvent ev)
{
UpdateAllCartridges(ev.Station);
}

private void OnVoteUpdated(VoteUpdatedEvent ev)
{
UpdateAllCartridges(ev.Station);
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

pro ref

Copy link
Contributor Author

Choose a reason for hiding this comment

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

looks like shit maybe make it better??

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Changes: UI Changes to UI files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants