Feature/#176 tooltips #180

nicolaskolbenschlag · 2023-01-19T07:27:20Z

No description provided.

Signed-off-by: nicolaskolbenschlag <nicolas.kolbenschlag@googlemail.com>

cypress · 2023-01-19T07:30:58Z

Test summary

28 • 5 • 0 • 0

Run details


Project	sosci-frontend
Status	Failed
Commit	`8e0e65c`
Started	Jan 23, 2023 3:10 PM
Ended	Jan 23, 2023 3:13 PM
Duration	03:13 💡
OS	Linux Ubuntu - 22.04
Browser	Chrome 109

View run in Cypress Dashboard ➡️

Failures are unavailable for this run. For more information, see the Cypress Dashboard

This comment has been generated by cypress-bot as a result of this project's GitHub integration settings. You can manage this integration in this project's settings in the Cypress Dashboard

PhlppKrmr

Very good job, only had some small comments :)

And very nice that you knew about sveltestrap tooltips!

PhlppKrmr · 2023-01-21T15:37:44Z

Apps/frontend/src/assets/mui_drop-down.svg

Also introduced a new icon for grounding 😆. Yours looks very good as well, guess we'll have to decide which one we'll take while merging.

PhlppKrmr · 2023-01-21T15:39:26Z

Apps/frontend/package.json

@@ -29,12 +29,13 @@
    "prettier": "2.8.0",
    "prettier-plugin-svelte": "^2.8.1",
    "sass": "^1.56.2",
-    "svelte": "^3.52.0",
+    "svelte": "^3.55.1",


Just a question. Does sveltestrap require/prefer a newer version of svelte or why did you update?

Thank you for the feedback!
It automaticallly updated when installing sveltestrap

Do we currently even have a mechanism like dependabot that regularly checks the version of our dependencies?

None that I know of

PhlppKrmr · 2023-01-21T15:45:22Z

Apps/frontend/src/components/DistributeOffsetButton.svelte

-        offsetY -= baseOffset;
+<div>
+  <button
+    id="btn-distribute"


Saw in sveltestrap that you can also use bind:this to specify the tooltip's target. Would imo look cleaner (especially when looking at the website in "Inspect" mode since there is no unnecessary id visible). Would this be too much of a change or can you adjust this?

Additionally, if you decide on keeping the id, does sveltestrap require button to be written as btn? Cause if it doesn't, I'd much prefer it to be written out to stay consistent with the rest of the code.

Yes but then we need additional variables on which we bind the buttons that we would not need elsewise. I changed it anyways.

PhlppKrmr · 2023-01-21T15:54:11Z

Apps/frontend/src/components/DistributeOffsetButton.svelte

-      if ($channelActivated[index]) {
-        $offsetAdjustment[index] = offsetY;
-        offsetY -= baseOffset;
+<div>


AFAIK you can actually remove the divs. Tried it out with the ground button and it still works as expected + the generated html looks cleaner (-> no "useless" div around the button since the Tooltip isn't rendered as an extra component anyway; that's what the aria-describedBy is for):

You're right. Removed the divs.

PhlppKrmr · 2023-01-21T15:59:00Z

Apps/frontend/src/components/GNDButton.svelte

+<div>
+  <button
+    id="btn-gnd"
+    class="icon-button mui-icon--drop-down"


Already adapted in #171, so you can decide if you want to keep it like this till the merge. This is the ground/gnd button, not drop-down.

PhlppKrmr · 2023-01-21T16:00:28Z

Apps/frontend/src/components/OnOffButton.svelte

+  <Tooltip target="btn-onoff" placement="bottom"
+    >{TOOLTIP_BUTTON_OFFOFF_BASE}{$osciEnabled
+      ? TOOLTIP_BUTTON_OFFOFF_OFF
+      : TOOLTIP_BUTTON_OFFOFF_ON}</Tooltip


Is this correctly linted?

Probably not :)
I will check it.

Apps/frontend/src/labels.js

Signed-off-by: nicolaskolbenschlag <nicolas.kolbenschlag@googlemail.com>

jenswaechtler

Please run the linter over it. At least two files (which you have not touched yourself) are changed for me. ChannelConfigControlPanel.svelte and ChannelConfigPopup.svelte
At least Reset and Distribute should be described even more precisely.

jenswaechtler · 2023-01-22T13:37:52Z

Apps/frontend/src/components/OnOffButton.svelte

+  {TOOLTIP_BUTTON_OFFOFF_BASE}{$osciEnabled
+    ? TOOLTIP_BUTTON_OFFOFF_OFF
+    : TOOLTIP_BUTTON_OFFOFF_ON}


Do we really want three labels for this?
Wouldn't one be good enough? And even if we want to describe the state, two labels would be fine instead of putting the two states together from three labels.

Changed it to two.

Apps/frontend/src/labels.js

jenswaechtler · 2023-01-22T13:42:57Z

Apps/frontend/package.json

@@ -29,12 +29,13 @@
    "prettier": "2.8.0",
    "prettier-plugin-svelte": "^2.8.1",
    "sass": "^1.56.2",
-    "svelte": "^3.52.0",
+    "svelte": "^3.55.1",


Do we currently even have a mechanism like dependabot that regularly checks the version of our dependencies?

jenswaechtler · 2023-01-22T13:45:36Z

Apps/frontend/src/labels.js

+export const TOOLTIP_BUTTON_DISTRIBUTE = "Distribute";
+export const TOOLTIP_BUTTON_RESET = "Reset";


Maybe a little more descriptive labels. Especially the ones that should contain a bit more information. What is distributed? What is reset?

Isn't reset self-explanatory, what text would you prefer elsewise?

I don't think so. After all, it doesn't reset everything like the sliders, for example. Maybe you should write "Cleans the canvas and stops plotting"?

Okay, sounds good. I'll change the label text :)

Signed-off-by: nicolaskolbenschlag <nicolas.kolbenschlag@googlemail.com>

jenswaechtler

The cypress tests are not going through.

Signed-off-by: nicolaskolbenschlag <nicolas.kolbenschlag@googlemail.com>

PhlppKrmr

Changes look good, the only thing I don't quite like are the extensively long tooltips for distribute and reset. I would've simply written "Distribute vertically" (/"Distribute") and "Reset channels" (/"Reset"). But that's only my opinion :)
Edit: Saw that you just changed it, do you have an opinion what you'd prefer?

Moreover, I for some reason have a problem with white and dark mode on this branch, but I don't know where those stem from. Might actually be related to your problems in #171. We can/should look into it next sprint.

rabbit-zero · 2023-01-23T16:02:21Z

I checked out this branch and somehow it looks like this in dark mode

nicolaskolbenschlag · 2023-01-23T16:03:46Z

Changes look good, the only thing I don't quite like are the extensively long tooltips for distribute and reset. I would've simply written "Distribute vertically" (/"Distribute") and "Reset channels" (/"Reset"). But that's only my opinion :) Edit: Saw that you just changed it, do you have an opinion what you'd prefer?

Moreover, I for some reason have a problem with white and dark mode on this branch, but I don't know where those stem from. Might actually be related to your problems in #171. We can/should look into it next sprint.

Thank you for the feedback.
I adapted the label texts according to Jens suggestion. Maybe we talk about it on wednesday and the POs might propose the final label texts. Chaning only the constant can be done later.
Regarding the dark-mode: I did not change anything related to the styling on this branch. I think this is related to the changes in dev.

PhlppKrmr · 2023-01-23T16:06:12Z

I checked out this branch and somehow it looks like this in dark mode

Same for me, we'll have to look into it next sprint.

nicolaskolbenschlag · 2023-01-23T16:09:10Z

I checked out this branch and somehow it looks like this in dark mode

Okay, thank you for the feedback.
I think this is because dev was merged into this branch yesterday.

nicolaskolbenschlag added 3 commits January 19, 2023 08:02

add sveltestrap, add tooltips for all general buttons

fb54c0d

Signed-off-by: nicolaskolbenschlag <nicolas.kolbenschlag@googlemail.com>

change gnd-btn icon

548870e

Signed-off-by: nicolaskolbenschlag <nicolas.kolbenschlag@googlemail.com>

move tooltip texts to labels.js #176

3d28670

Signed-off-by: nicolaskolbenschlag <nicolas.kolbenschlag@googlemail.com>

nicolaskolbenschlag requested review from PhlppKrmr and motschel123 January 20, 2023 17:04

PhlppKrmr requested changes Jan 21, 2023

View reviewed changes

add requested changes for #176

24233e6

Signed-off-by: nicolaskolbenschlag <nicolas.kolbenschlag@googlemail.com>

nicolaskolbenschlag requested a review from PhlppKrmr January 22, 2023 07:57

jenswaechtler suggested changes Jan 22, 2023

View reviewed changes

jandegen and others added 3 commits January 22, 2023 18:57

Merge branch 'dev' into feature/#176-tooltips

75cb2c5

#176 requested change label variable naming

30f21e0

Signed-off-by: nicolaskolbenschlag <nicolas.kolbenschlag@googlemail.com>

#176 lint two additional files

9935659

Signed-off-by: nicolaskolbenschlag <nicolas.kolbenschlag@googlemail.com>

nicolaskolbenschlag requested a review from jenswaechtler January 23, 2023 09:04

jenswaechtler approved these changes Jan 23, 2023

View reviewed changes

change labels #176

8e0e65c

Signed-off-by: nicolaskolbenschlag <nicolas.kolbenschlag@googlemail.com>

PhlppKrmr approved these changes Jan 23, 2023

View reviewed changes

nicolaskolbenschlag merged commit ebd682f into dev Jan 23, 2023

This was referenced Jan 25, 2023

sprint-12-release-candidate #193

Merged

sprint-12-release-candidate #194

Merged

sprint-12-release #195

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/#176 tooltips #180

Feature/#176 tooltips #180

nicolaskolbenschlag commented Jan 19, 2023

cypress bot commented Jan 19, 2023 •

edited

Loading

PhlppKrmr left a comment •

edited

Loading

PhlppKrmr Jan 21, 2023

PhlppKrmr Jan 21, 2023

nicolaskolbenschlag Jan 22, 2023

jenswaechtler Jan 22, 2023

PhlppKrmr Jan 23, 2023

PhlppKrmr Jan 21, 2023

nicolaskolbenschlag Jan 22, 2023

PhlppKrmr Jan 21, 2023

nicolaskolbenschlag Jan 22, 2023 •

edited

Loading

PhlppKrmr Jan 21, 2023

PhlppKrmr Jan 21, 2023

nicolaskolbenschlag Jan 22, 2023

jenswaechtler left a comment

jenswaechtler Jan 22, 2023

nicolaskolbenschlag Jan 23, 2023

jenswaechtler Jan 22, 2023

jenswaechtler Jan 22, 2023

nicolaskolbenschlag Jan 23, 2023

jenswaechtler Jan 23, 2023

nicolaskolbenschlag Jan 23, 2023

jenswaechtler left a comment

PhlppKrmr left a comment •

edited

Loading

rabbit-zero commented Jan 23, 2023

nicolaskolbenschlag commented Jan 23, 2023

PhlppKrmr commented Jan 23, 2023

nicolaskolbenschlag commented Jan 23, 2023

		export const TOOLTIP_BUTTON_DISTRIBUTE = "Distribute";
		export const TOOLTIP_BUTTON_RESET = "Reset";

Feature/#176 tooltips #180

Feature/#176 tooltips #180

Conversation

nicolaskolbenschlag commented Jan 19, 2023

cypress bot commented Jan 19, 2023 • edited Loading

Test summary

Run details

PhlppKrmr left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicolaskolbenschlag Jan 22, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jenswaechtler left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jenswaechtler left a comment

Choose a reason for hiding this comment

PhlppKrmr left a comment • edited Loading

Choose a reason for hiding this comment

rabbit-zero commented Jan 23, 2023

nicolaskolbenschlag commented Jan 23, 2023

PhlppKrmr commented Jan 23, 2023

nicolaskolbenschlag commented Jan 23, 2023

cypress bot commented Jan 19, 2023 •

edited

Loading

PhlppKrmr left a comment •

edited

Loading

nicolaskolbenschlag Jan 22, 2023 •

edited

Loading

PhlppKrmr left a comment •

edited

Loading