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

TGUI TEG #1965

Merged
merged 15 commits into from Sep 3, 2020
Merged

TGUI TEG #1965

merged 15 commits into from Sep 3, 2020

Conversation

Sovexe
Copy link
Contributor

@Sovexe Sovexe commented Aug 27, 2020

About the PR

TGUI TEG

edit: Now featuring our first chart!
image

the misspelling on inlet in these images has been corrected
imageimage
image

Why's this needed?

Consistency with the rest of the new TGUI engineering UI's

Changelog

(u)Sovexe:
(+)Converted TEG interface to TGUI

@keywordlabeler keywordlabeler bot added the C-Feature A new feature or enhancements to existing features label Aug 27, 2020
@boring-cyborg boring-cyborg bot added the A-UI Modifies UI in some way. Automatically applied on a change to tgui/ label Aug 27, 2020
@Sovexe Sovexe changed the title Tgui teg Tgui TEG Aug 27, 2020
@Sovexe Sovexe changed the title Tgui TEG TGUI TEG Aug 27, 2020
@ZeWaka
Copy link
Member

ZeWaka commented Aug 27, 2020

changelog

@Sovexe
Copy link
Contributor Author

Sovexe commented Aug 27, 2020

changelog

fixed

@@ -19,6 +19,7 @@
#define KELVIN *1
#define MOLES *1
#define CANDELAS *1
#define PASCALS *1
Copy link
Member

@ZeWaka ZeWaka Aug 27, 2020

Choose a reason for hiding this comment

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

technically *(1 KILOGRAM)/(1 METER * ((0.1 SECONDS)**2)) but I'll let this slide

@Sovexe
Copy link
Contributor Author

Sovexe commented Aug 30, 2020

I added a chart
image

code/WorkInProgress/TempEngine.dm Outdated Show resolved Hide resolved
Co-authored-by: ZeWaka <zewakagamer@gmail.com>
@github-actions github-actions bot added the S-Merge-Conflict Applied and removed when a PR has or no longer has a merge conflict label Sep 2, 2020
Comment on lines +448 to +459
if(src.circ1)
data["hotCircStatus"] = src.circ1
data["hotInletTemp"] = src.circ1.air1.temperature
data["hotOutletTemp"] = src.circ1.air2.temperature
data["hotInletPres"] = MIXTURE_PRESSURE(src.circ1.air1) KILO PASCALS
data["hotOutletPres"] = MIXTURE_PRESSURE(src.circ1.air2) KILO PASCALS
else
data["hotCircStatus"] = null
data["hotInletTemp"] = 0
data["hotOutletTemp"] = 0
data["hotInletPres"] = 0
data["hotOutletPres"] = 0
Copy link
Contributor

Choose a reason for hiding this comment

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

Could also consider doing as a nested list, then just having null if src.circ1 isn't set. Would mean handling a little more in the UI, but I don't think that's necessarily a bad thing.

This is absolutely simple and readable enough that I think the if/else is absolutely fine here. Educating for more complex cases.

Comment on lines +15 to +28
const {
output,
history,
hotCircStatus,
hotInletTemp,
hotOutletTemp,
hotInletPres,
hotOutletPres,
coldCircStatus,
coldInletTemp,
coldOutletTemp,
coldInletPres,
coldOutletPres,
} = data;
Copy link
Contributor

Choose a reason for hiding this comment

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

For reference, if using the nested object thing as mentioned in the DM file, this would look like:

  const {
    output,
    history,
    hot: {
      circStatus: hotCircStatus = null,
      inletTemp: hotInletTemp = 0,
      outletTemp: hotOutletTemp = 0,
      inletPres: hotInletPres = 0,
      outletPres: hotOutletPres = 0,
    } = {},
    cold: {
      circStatus: coldCircStatus = null,
      inletTemp: coldInletTemp = 0,
      outletTemp: coldOutletTemp = 0,
      inletPres: coldInletPres = 0,
      outletPres: coldOutletPres = 0,
    } = {},
  } = data;

Comment on lines +45 to +46
strokeColor="rgba(1, 184, 170, 1)"
fillColor="rgba(1, 184, 170, 0.25)" />
Copy link
Contributor

Choose a reason for hiding this comment

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

Fairly arbitrary colors. They look nice here, but should probably be moved to (and then drawn from) the theme.

tgui/packages/tgui/interfaces/TEG.js Outdated Show resolved Hide resolved
tgui/packages/tgui/interfaces/TEG.js Outdated Show resolved Hide resolved
tgui/packages/tgui/interfaces/TEG.js Outdated Show resolved Hide resolved
tgui/packages/tgui/interfaces/TEG.js Outdated Show resolved Hide resolved
@Sovexe Sovexe merged commit 3a15c48 into goonstation:master Sep 3, 2020
github-actions bot pushed a commit that referenced this pull request Sep 3, 2020
@Sovexe Sovexe deleted the tgui-teg branch September 3, 2020 14:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-UI Modifies UI in some way. Automatically applied on a change to tgui/ C-Feature A new feature or enhancements to existing features S-Merge-Conflict Applied and removed when a PR has or no longer has a merge conflict
Projects
Development

Successfully merging this pull request may close these issues.

None yet

3 participants