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

Adds Custom Announcement Dividers #79071

Merged
merged 22 commits into from
Oct 20, 2023
Merged

Adds Custom Announcement Dividers #79071

merged 22 commits into from
Oct 20, 2023

Conversation

san7890
Copy link
Member

@san7890 san7890 commented Oct 18, 2023

This ports a whole bunch of various PRs and commits from https://github.com/effigy-se/effigy-se , with heavy refactoring to keep it fresh for /tg/'s code standards.

About The Pull Request

The whole slew of announcement touchups lately (as in #78995 (37db1ec) / #79052 (12308db)) have made me realize how much this stuff sucks. The author of these new spans was advertising these in coding general, so I sat down and coded it. Look at the spans, they're much nicer than what we had going on:

(ignore the capitalized alert status names, this was removed)

Dark Mode

image
image

Light Mode

image
image

This PR also features

  • Major announcement code handling cleanup and refactor! There was a lot of copypasta so let's distill it all down into one proc
  • Better cacheing! We were doing a shit load of new string generation needlessly! That's fixed now.
  • Better string concatenation! Lists are better for string tree reasons. It still works just as well, as you can see from the screenshots above. Best of all, no fucking <br> dogshit everywhere!
  • We don't use string equivalency in order to figure out the "type" of an announcement. It's all defines now. This was a bonus that I just coded in since it irritated me.
  • Minor spellcheck of "announcement".
  • All of our HTML string mangling stuff is now all span macros! I love macros.

Why It's Good For The Game

In the same vein of adding examine blocks (#67937 (b864589)) because old examinations tended to blend in with the chat and everything chat-wise used to suck really hard- I think this is a really nice way to draw attention to announcements in the chat box without needing a shit load of line breaks that just really look ugly and have no real consistency. You can look at the PRs/commits I linked above for an idea of just how ugly it could be getting.

I haven't audited every announcement in this PR, we can tweak this down the line.

Changelog

🆑 LT3, san7890
add: Announcements have gotten a fresh coat of paint! They should be popping with splendid new colors and should have a lot less ugly linebreaks, while still managing to keep your attention at the screen.
/:cl:

I know we didn't need to port all the CSS themes but I added them anyways in case admins wanna have some fun.
There can probably be more code improvements, just figured I'd crack it out while I had time.
The colors also seem fine, let me know if we need more redness or something. It's okay for stuff to be toned down a bit imo, but that should be done after a hot second.

at least i can atomize this out
pretty much just from effigy no. 34 and other widespread commits

chat_alert_2 is now aliased to chat_alert_default
or whatever they are really
spellchecks and level_announce(), more spans
@san7890 san7890 added the Refactor Makes the code harder to read label Oct 18, 2023
@tgstation-server tgstation-server added Feature Exposes new bugs in interesting ways UI We make the game less playable, but with round edges labels Oct 18, 2023
tgui/packages/tgui-panel/styles/tgchat/chat-dark.scss Outdated Show resolved Hide resolved
code/__HELPERS/priority_announce.dm Outdated Show resolved Hide resolved
san7890 and others added 3 commits October 18, 2023 08:56
Co-authored-by: lessthanthree <83487515+lessthnthree@users.noreply.github.com>
we'll keep the old one for the time being
Comment on lines 969 to 986
.chat_alert_default {
color: #ffffff;
padding: 0.5em 0.5em;
margin-bottom: 0.5em;
box-shadow: none;
font-weight: bold;
border: 1px solid #292929;
margin: 0.5em 0 0.5em 0;
padding: 0.5em 0.75em;
background-color: #252525;
background-image: repeating-linear-gradient(
-45deg,
transparent,
transparent 10px,
#292929 10px,
#292929 20px
);
}
Copy link
Member

Choose a reason for hiding this comment

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

I don't think these look great being the background of the announcement system.
Have you considered trying out a border-like system? i.e. the gradient background only acts as a border and the inside is a solid box akin to examine text.
image

I think it's fine overall, but text layered over patterns tend to be slightly harder to read

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll ponder on this but I think this can also be done later. If I have time I'll see if I can confangle a demo but I'm no CSS ninja

Copy link
Member Author

Choose a reason for hiding this comment

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

yeah implementing something like this is beyond the reach of my skill. it doesn't seem possible to do it in a really clean way from what I was reading about this stuff online, especially now that we've macroized the css classes per the other review. this is a challenge for another day.

tgui/packages/tgui-panel/styles/tgchat/chat-dark.scss Outdated Show resolved Hide resolved
if(target.client.prefs.read_preference(/datum/preference/toggle/sound_announcements))
SEND_SOUND(target, sound_to_play)
if(isnull(sender_override))
if(length(title) > 0)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if(length(title) > 0)
if(length(title))

Is it even possible to have negative text length?

Copy link
Member Author

@san7890 san7890 Oct 20, 2023

Choose a reason for hiding this comment

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

No but it's a codehint pattern that tends to be more explicit. I critiqued some other guys with that same point (implying there's a potential negative text length) over this and they said it just felt better to be more explicit (since we're technically treating a non-boolean value as a boolean if we remove the > 0 portion, as length() returns an integer), which I conceded

I dont care either way

switch(type)
if(ANNOUNCEMENT_TYPE_PRIORITY)
header = MAJOR_ANNOUNCEMENT_TITLE("Priority Announcement")
if(length(title) > 0)
Copy link
Member

Choose a reason for hiding this comment

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

Ditto

@Ghommie Ghommie merged commit 30bac3a into tgstation:master Oct 20, 2023
20 checks passed
github-actions bot added a commit that referenced this pull request Oct 20, 2023
comfyorange added a commit that referenced this pull request Oct 20, 2023
Jolly-66 pushed a commit to TaleStation/TaleStation that referenced this pull request Oct 25, 2023
Original PR: tgstation/tgstation#79071
-----
This ports a whole bunch of various PRs and commits from
https://github.com/effigy-se/effigy-se , with heavy refactoring to keep
it fresh for /tg/'s code standards.

## About The Pull Request

The whole slew of announcement touchups lately (as in #78995
(37db1ecbf8d7ceeaeb134c56b403339555f44cea) / #79052
(12308dbd3dae702a1c7b9f7f80d7859aea085c82)) have made me realize how
much this stuff sucks. The author of these new spans was advertising
these in coding general, so I sat down and coded it. Look at the spans,
they're much nicer than what we had going on:

(ignore the capitalized alert status names, this was removed)

<details>
<summary>Dark Mode</summary>


![image](https://github.com/tgstation/tgstation/assets/34697715/107b8efb-b7a1-41ff-9d16-358c4dc3738d)

![image](https://github.com/tgstation/tgstation/assets/34697715/9e730dfe-7ba3-4edd-96bb-0630fe5e85cf)
</details>

<details>
<summary>Light Mode</summary>


![image](https://github.com/tgstation/tgstation/assets/34697715/57f642f9-ee17-4b16-8027-00a9350e9059)

![image](https://github.com/tgstation/tgstation/assets/34697715/b28b7f49-fd4f-420a-9313-e16b9781c07c)
</details>

This PR also features

* Major announcement code handling cleanup and refactor! There was a lot
of copypasta so let's distill it all down into one proc
* Better cacheing! We were doing a shit load of new string generation
needlessly! That's fixed now.
* Better string concatenation! Lists are better for string tree reasons.
It still works just as well, as you can see from the screenshots above.
Best of all, no fucking `<br>` dogshit everywhere!
* We don't use string equivalency in order to figure out the "type" of
an announcement. It's all defines now. This was a bonus that I just
coded in since it irritated me.
* Minor spellcheck of "announcement".
* All of our HTML string mangling stuff is now all span macros! I love
macros.

## Why It's Good For The Game

In the same vein of adding examine blocks (#67937
(b864589)) because old examinations
tended to blend in with the chat and everything chat-wise used to suck
really hard- I think this is a really nice way to draw attention to
announcements in the chat box without needing a shit load of line breaks
that just really look ugly and have no real consistency. You can look at
the PRs/commits I linked above for an idea of just how ugly it could be
getting.

I haven't audited every announcement in this PR, we can tweak this down
the line.

## Changelog

:cl: LT3, san7890
add: Announcements have gotten a fresh coat of paint! They should be
popping with splendid new colors and should have a lot less ugly
linebreaks, while still managing to keep your attention at the screen.
/:cl:

I know we didn't need to port all the CSS themes but I added them
anyways in case admins wanna have some fun.
There can probably be more code improvements, just figured I'd crack it
out while I had time.
The colors also seem fine, let me know if we need more redness or
something. It's okay for stuff to be toned down a bit imo, but that
should be done after a hot second.

---------

Co-authored-by: san7890 <the@san7890.com>
Co-authored-by: lessthanthree <83487515+lessthnthree@users.noreply.github.com>
nevimer pushed a commit to Bubberstation/Bubberstation that referenced this pull request Dec 6, 2023
## About The Pull Request

Reverts the CSS styling for announcements back to TG/Effigy original as
in tgstation/tgstation#79071,
tgstation/tgstation#79236.

## Why It's Good For The Game

There's no technical need to override this, the announcements overhaul
should be implemented as designed. Bubber doesn't need SR's override.

## Changelog

:cl: LT3
code: Announcement CSS is reverted to TG default
/:cl:
Gboster-0 pushed a commit to Gboster-0/Monkestation2.0 that referenced this pull request Jan 7, 2024
This ports a whole bunch of various PRs and commits from
https://github.com/effigy-se/effigy-se , with heavy refactoring to keep
it fresh for /tg/'s code standards.

The whole slew of announcement touchups lately (as in tgstation#78995
(37db1ec) / tgstation#79052
(12308db)) have made me realize how
much this stuff sucks. The author of these new spans was advertising
these in coding general, so I sat down and coded it. Look at the spans,
they're much nicer than what we had going on:

(ignore the capitalized alert status names, this was removed)

<details>
<summary>Dark Mode</summary>

![image](https://github.com/tgstation/tgstation/assets/34697715/107b8efb-b7a1-41ff-9d16-358c4dc3738d)

![image](https://github.com/tgstation/tgstation/assets/34697715/9e730dfe-7ba3-4edd-96bb-0630fe5e85cf)
</details>

<details>
<summary>Light Mode</summary>

![image](https://github.com/tgstation/tgstation/assets/34697715/57f642f9-ee17-4b16-8027-00a9350e9059)

![image](https://github.com/tgstation/tgstation/assets/34697715/b28b7f49-fd4f-420a-9313-e16b9781c07c)
</details>

This PR also features

* Major announcement code handling cleanup and refactor! There was a lot
of copypasta so let's distill it all down into one proc
* Better cacheing! We were doing a shit load of new string generation
needlessly! That's fixed now.
* Better string concatenation! Lists are better for string tree reasons.
It still works just as well, as you can see from the screenshots above.
Best of all, no fucking `<br>` dogshit everywhere!
* We don't use string equivalency in order to figure out the "type" of
an announcement. It's all defines now. This was a bonus that I just
coded in since it irritated me.
* Minor spellcheck of "announcement".
* All of our HTML string mangling stuff is now all span macros! I love
macros.

In the same vein of adding examine blocks (tgstation#67937
(b864589)) because old examinations
tended to blend in with the chat and everything chat-wise used to suck
really hard- I think this is a really nice way to draw attention to
announcements in the chat box without needing a shit load of line breaks
that just really look ugly and have no real consistency. You can look at
the PRs/commits I linked above for an idea of just how ugly it could be
getting.

I haven't audited every announcement in this PR, we can tweak this down
the line.

:cl: LT3, san7890
add: Announcements have gotten a fresh coat of paint! They should be
popping with splendid new colors and should have a lot less ugly
linebreaks, while still managing to keep your attention at the screen.
/:cl:

I know we didn't need to port all the CSS themes but I added them
anyways in case admins wanna have some fun.
There can probably be more code improvements, just figured I'd crack it
out while I had time.
The colors also seem fine, let me know if we need more redness or
something. It's okay for stuff to be toned down a bit imo, but that
should be done after a hot second.

Co-Authored-By: lessthanthree <83487515+lessthnthree@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature Exposes new bugs in interesting ways Refactor Makes the code harder to read UI We make the game less playable, but with round edges
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants