Skip to content

Conversation

@ribalba
Copy link
Member

@ribalba ribalba commented Dec 1, 2025

This PR adds the functionality to add 0..n variables to a run. Tried to make it look nice and not just a textbox.

Screenshot 2025-12-01 at 22 23 32

@ribalba ribalba requested a review from ArneTR December 1, 2025 21:24
@greptile-apps
Copy link
Contributor

greptile-apps bot commented Dec 1, 2025

Skipped: This PR does not contain any of your configured keywords: (greptile-this-will-never-be-only-manual)

@github-actions
Copy link

github-actions bot commented Dec 2, 2025

Eco CI Output - Old Energy Estimation

Eco CI Output [RUN-ID: 19838008509]:

🌳 CO2 Data:
City: CONSTANT, Lat: , Lon:
IP:
CO₂ from energy is: 1.528159710 g
CO₂ from manufacturing (embodied carbon) is: 0.449155262 g
Carbon Intensity for this location: 231 gCO₂eq/kWh
SCI: 1.977315 gCO₂eq / pipeline run emitted


Total cost of whole PR so far:

Label🖥 avg. CPU utilization [%]🔋 Total Energy [Joules]🔌 avg. Power [Watts]Duration [Seconds]
Measurement #128.93566615.414.201574.25
Total Run28.946615.414.201574.25
Additional overhead from Eco CIN/A17.374.254.09

@ribalba
Copy link
Member Author

ribalba commented Dec 2, 2025

I had to run the tests 2 times so that it would pass. No idea what is happening here.

@ArneTR
Copy link
Member

ArneTR commented Dec 3, 2025

Seems to work as intended. Also the code LGTM.

Visually I would argue for some overhaul:

  • The layout crashes on mobile. See screenshot
  • I think the button to "add variables" should be on the right. it is quite confusing above the submit button
  • I would remove the <div class="ui basic segment"> container. I understand that it is to intend the variables, but this feels more confusing than helpful to me
  • In order to increase visibility for the option I would always show one entry box for the variable which can be left optionally empty and indicate this in the placeholder. screenshot attached (I pushed this as commit which can be reverted after discussion)

Mobile problems

Screenshot 2025-12-03 at 11 09 25 AM Screenshot 2025-12-03 at 11 09 50 AM

Design Idea

Screenshot 2025-12-03 at 11 17 31 AM

@ribalba
Copy link
Member Author

ribalba commented Dec 4, 2025

So it looks like this now:

Screenshot 2025-12-04 at 14 18 33

Mobile:

Screenshot 2025-12-04 at 14 19 30

Feedback

@ArneTR
Copy link
Member

ArneTR commented Dec 4, 2025

The columns still create a padding when on mobile left and right. However I think this is a minor style issue and I expect not many people using it on mobile.
It should mostly be readable and non broken. So this PR LGTM.

Can merge after tests run successfully

@github-actions
Copy link

github-actions bot commented Dec 4, 2025

Eco CI Output - Old Energy Estimation

Eco CI Output [RUN-ID: 19930426278]:

🌳 CO2 Data:
City: CONSTANT, Lat: , Lon:
IP:
CO₂ from energy is: 1.541841840 g
CO₂ from manufacturing (embodied carbon) is: 0.452787307 g
Carbon Intensity for this location: 231 gCO₂eq/kWh
SCI: 1.994629 gCO₂eq / pipeline run emitted


Total cost of whole PR so far:

Label🖥 avg. CPU utilization [%]🔋 Total Energy [Joules]🔌 avg. Power [Watts]Duration [Seconds]
Measurement #128.96536674.644.211586.98
Total Run28.976674.644.211586.98
Additional overhead from Eco CIN/A17.404.264.08

@github-actions
Copy link

github-actions bot commented Dec 4, 2025

Eco CI Output [RUN-ID: 19931440629]:

🌳 CO2 Data:
City: CONSTANT, Lat: , Lon:
IP:
CO₂ from energy is: 1.543973970 g
CO₂ from manufacturing (embodied carbon) is: 0.448033979 g
Carbon Intensity for this location: 231 gCO₂eq/kWh
SCI: 1.992008 gCO₂eq / pipeline run emitted


Total cost of whole PR so far:

Label🖥 avg. CPU utilization [%]🔋 Total Energy [Joules]🔌 avg. Power [Watts]Duration [Seconds]
Measurement #130.20076683.874.261570.32
Total Run30.206683.874.261570.32
Additional overhead from Eco CIN/A16.994.244.01

@ribalba ribalba merged commit 8f34462 into main Dec 4, 2025
1 check passed
@ribalba ribalba deleted the adds-vars-frontend branch December 4, 2025 14:24
ArneTR added a commit that referenced this pull request Dec 4, 2025
* main:
  Refactoring of unneeded double querySelector
  Adds variables to the submit form (#1459)
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