-
Notifications
You must be signed in to change notification settings - Fork 12
Conversation
Deploy preview for catwalk-qlikcore ready! Built with commit 178497b |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice, some minor comments :)
src/components/guide-steps.jsx
Outdated
@@ -6,7 +6,7 @@ const steps = [ | |||
<div> | |||
<h2>Welcome to catwalk!</h2> | |||
<p> | |||
catwalk lets you explore your data model to gain | |||
Catwalk lets you explore your data model to gain |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
:(
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know.. but the other team mates found it strange to begin a sentence with lower case, even though the name is 'catwalk'.
src/components/guide-steps.jsx
Outdated
content: ( | ||
<div> | ||
<p> | ||
This shows the association between two fields, with basic frequency information on each end of the association line (1, 0/1 or *). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we use <code>
elsewhere, perhaps 1
, 0/1
, and *
should be too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch!
|
||
useClickOutside(selfRef, true, () => { | ||
chooseColumn(null); | ||
useClickOutside(selfRef, closeOnClickOutside(), () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not completely folloing why closeOnClickOutside
needs to be a fn instead of bool if we have the value passed in from app.jsx
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
well.. guideRef.current (in app.jsx) is undefined when the components are initialized, that's why I made it a function from the beginning.
If not using a function we need to update a state in App that is dependent on Guide..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good!
Small comment:
In the "Selections" step. Pressing the X to remove a selection is mentioned. If this is done the guide is backing up to the previous step. Consider disabling the X in this step.
The Selections step should not be able to "click through". Then there is no need for disabling the 'X'. |
Improved the guide according to feedback from the manual test session.