Skip to content

Conversation

@CKegel
Copy link
Contributor

@CKegel CKegel commented Mar 13, 2023

Changed style of info to all capital letters for clarity.
Before:
Before
After:
After

@CKegel CKegel requested review from ddbruce and lramos15 as code owners March 13, 2023 23:30
@ddbruce
Copy link
Member

ddbruce commented Mar 13, 2023

Add before and after screenshots to the PR comments

@CKegel
Copy link
Contributor Author

CKegel commented Mar 13, 2023

Before and After pictures have been added

@lramos15
Copy link
Contributor

This seems unnecessarily bold, especially given the rest of the UI is in normal casing. I understand maybe the determinant but does everything need to be caps?

@ddbruce
Copy link
Member

ddbruce commented Mar 14, 2023

https://en.wikipedia.org/wiki/All_caps#Readability

https://uxmovement.com/content/all-caps-hard-for-users-to-read/

But when your message involves reading, don’t force users to read words with bad shape contrast.

Edit: I could totally get behind the determinant alone, but I'd have to be convinced pretty hard that the rest of it is more readable with caps

@CKegel
Copy link
Contributor Author

CKegel commented Mar 14, 2023

I spoke with the line side officers and the dev coordinator, they(and I) agree and I'm going to change it so each word is capitalized, with the determinant in uppercase. The reason they requested uppercase was to hide dispatch inconsistencies regarding RPI. Would you be opposed to adding some regex that replaced mis-cased versions of Rpi with RPI?

@CKegel
Copy link
Contributor Author

CKegel commented Mar 14, 2023

Here is the new alert with the following input
Determinant: "Charlie"
Complaint: "Abdominal pain"
Location: "Rpi - Sage Labs"
image

@lramos15
Copy link
Contributor

Would you be opposed to adding some regex that replaced mis-cased versions of Rpi with RPI?

No that seems fine

Copy link
Member

@ddbruce ddbruce left a comment

Choose a reason for hiding this comment

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

Looks fine, but let's take this time to clean up the code such that #dispatch is in the dispatch CSS file and not in both the light and dark mode files. It is the same CSS, right?

@CKegel
Copy link
Contributor Author

CKegel commented Mar 14, 2023

Looks fine, but let's take this time to clean up the code such that #dispatch is in the dispatch CSS file and not in both the light and dark mode files. It is the same CSS, right?

The code has been cleaned, it is the same css

Copy link
Member

@ddbruce ddbruce left a comment

Choose a reason for hiding this comment

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

lgtm, thanks @CKegel!

@ddbruce ddbruce merged commit 9e29c58 into techinems:dev Mar 14, 2023
ddbruce added a commit that referenced this pull request Mar 21, 2023
* Master -> dev (#44)

Co-authored-by: Logan Ramos <lramos15@gmail.com>

* Add CAD Display from Herald (#65)

* Restructured Index.html and added endpoint for herald to push dispatch info to

* Styled Dispatch alerts, set dispatch alert duration to three minutes, and fixed odds and ends

* add dev env

* add dev workflow

* match database fields

* add css file to index.html

* this is what happens when you copy and paste a line

* eslint

* work on dispatch styling

* more styling

* add dispatch time

* fix dispatch time text

* Style Changes for Headsup Dispatch (#66)

* Changed Styling to capitalize dispatch info by request of RPIA dev-team coordinator

* Capitalize the start of words and Uppercase the determinant in Dispatch Alerts

* Moved Dispatch style to style-dispatch.scss

* BugFix: Fix issues with dispatches that occur within 3 minutes of each other (#68)

* Changed Styling to capitalize dispatch info by request of RPIA dev-team coordinator

* Capitalize the start of words and Uppercase the determinant in Dispatch Alerts

* Moved Dispatch style to style-dispatch.scss

* Fixed issues with dispatches that occur within 3 minutes of each other

* Correct Logic Errors in ClearDispatch Timeout

* Added authentication token for herald dispatch

* 1.5.0

* Clean Code and Add Admin Authentication (#70)

* Spell checking and code adjustments

* Added token authentication to admin POST endpoints

* restructure admin authentication

* fix double-dispatch clearing time issue

* update build status badge

---------

Co-authored-by: Logan Ramos <lramos15@gmail.com>
Co-authored-by: Christian <57967583+CKegel@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants