-
Notifications
You must be signed in to change notification settings - Fork 4
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
Refactor HelpComponent #418
Conversation
src/services/HelpService.js
Outdated
} else { | ||
pathWithExtension = `${path}.json` | ||
if (this.HelpCache[path] != undefined) { | ||
if (this.HelpCache[path] != "") { |
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.
@Aringoaway I was thinking, do we really need this condition? Seems redundant to me, because we are setting an empty string as a value to the cache, if the content has not been retrieved
asab-webui/src/services/HelpService.js
Line 45 in 8549b9a
this.HelpCache[path] = ""; |
So I suggest a refactoring
if (this.HelpCache[path] != undefined) {
this.App.Store.dispatch({
type: HELP_CONTENT,
content: this.HelpCache[path]
});
return;
}
helpFrame.mp4 |
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.
@Aringoaway Can I see also video with network in console? I want to see, if the iframe url is triggered only when ?
button is pressed
src/services/HelpService.js
Outdated
@@ -8,31 +8,11 @@ export default class HelpService extends Service { | |||
} | |||
|
|||
async setData(path) { |
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.
@Aringoaway It does not have to be async
since there is no async action
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.
@Aringoaway btw. setData
is not a good name for it, since all you do is setting the path
Otherwise it looks good! |
callFrame.mp4 |
src/containers/Application.js
Outdated
@@ -445,14 +445,14 @@ class Application extends Component { | |||
|
|||
addHelpButton(path) { | |||
useEffect(() => { | |||
this.HelpService.setData(path); | |||
this.HelpService.setPath(path); |
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.
@Aringoaway Do we actually need the HelpService then? we can store the path directly to the store I assume
this.Store.dispatch({
type: HELP_CONTENT,
path: path
});
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 agree. That's a great idea.
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.
@Aringoaway And this https://github.com/TeskaLabs/asab-webui/pull/418/files#diff-31d3784ef11656fbccbce9b6b554c572b43dfad75c897ef7fa510ba7a443735fR98 should be refactored to this.ReduxService.addReducer("header", headerReducer);
src/containers/Application.js
Outdated
@@ -447,14 +445,17 @@ class Application extends Component { | |||
|
|||
addHelpButton(path) { | |||
useEffect(() => { | |||
this.HelpService.setData(path); | |||
this.Store.dispatch({ | |||
type: HELP_CONTENT, |
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.
@Aringoaway type should be SET_HELP_CONTENT_PATH
or something similar as we spoke of about on standup
src/containers/Application.js
Outdated
this.HelpService.setData(path); | ||
this.Store.dispatch({ | ||
type: HELP_CONTENT, | ||
path: path |
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.
@Aringoaway should be somethng like helpContentPath: path
src/containers/Header/HelpButton.js
Outdated
@@ -10,8 +10,9 @@ export default function HelpButton() { | |||
|
|||
const [modal, setModal] = useState(false); | |||
|
|||
const content = useSelector(state => state?.helpButton.content); | |||
if ((content == undefined) || (content == "")) return null; | |||
const path = useSelector(state => state?.helpButton.path); |
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.
@Aringoaway should be const path = useSelector(state => state?.header.helpContentPath);
as we spoke of on standup
src/containers/Header/reducer.js
Outdated
@@ -1,14 +1,14 @@ | |||
import { HELP_CONTENT } from '../../actions'; | |||
|
|||
const initialState = { | |||
content: "" | |||
path: "" |
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.
@Aringoaway helpContentPath
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.
@aringocode Minor thing, otherwise good!
src/containers/Application.js
Outdated
@@ -447,14 +445,17 @@ class Application extends Component { | |||
|
|||
addHelpButton(path) { | |||
useEffect(() => { | |||
this.HelpService.setData(path); | |||
this.Store.dispatch({ | |||
type: HELP_CONTENT, |
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.
@aringocode Set type to SET_HELP_PATH
src/containers/Header/reducer.js
Outdated
@@ -1,4 +1,4 @@ | |||
import { HELP_CONTENT, SET_BREADCRUMB_NAME } from '../../actions'; | |||
import { SET_HELP_PATH, SET_BREADCRUMB_NAME } from '../../actions'; | |||
|
|||
const initialState = { | |||
content: "", |
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.
@aringocode Shouldnt here be some default content as we spoke of after the standup? Meaning https://docs.teskalabs.com/logman.io/ or https://docs.teskalabs.com/logman.io/user/ or so...
Btw. can you please rename content
to path
? Since its not content any longer, but path/url
@Pe5h4 Can you review pls? lmio-webui-HelpButton.mp4seacat-admin-webui-HelpButton.mp4 |
src/containers/Header/HelpButton.js
Outdated
const title = useSelector(state => state.config?.title); | ||
|
||
useEffect(() => { | ||
if (location.pathname.includes("auth") || (title == "SeaCat Admin")) { |
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.
@aringocode I dont think this is correct. We want to make it more variable in case of what URL we are going to insert. So what we want to pass is the whole url, meaning https://docs.teskalabs.com/logman.io/user/whatever
since the help component may be used also for some external documentation. I suggest to remove all logman.io
/ auth
/Seacat Admin
specifications. If nothing will be passed, then we dont display the HelpButton
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.
You will also get rid of this useEffect and other unnecessary code about switching the paths
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.
@Pe5h4 ok, I refactored it
helpButton.mp4
doc/help-button.md
Outdated
|
||
#### Example code | ||
|
||
```javascript | ||
export function Container(props) { | ||
props.app.addHelpButton("Path/to/help-content"); | ||
props.app.addHelpButton("Path/part/to/documentation"); |
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.
@aringocode this should be
props.app.addHelpButton("https://docs.teskalabs.com/path/to/documentation");
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.
@aringocode Alright, looks good, only minor change request
src/containers/Header/HelpButton.js
Outdated
</Button> | ||
</CardHeader> | ||
<CardBody> | ||
<iframe className="help-iframe" src={`https://docs.teskalabs.com/logman.io/user/${path}`} title=""/> |
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.
@aringocode Here you want to have just
<iframe className="help-iframe" src={path} />
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.
@Pe5h4 Ohhh...., I understand) can you review again pls?))
src/containers/Application.js
Outdated
this.HelpService.setData(path); | ||
this.Store.dispatch({ | ||
type: SET_HELP_PATH, | ||
path: path |
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.
@aringocode Alright, sorry for requesting changes all over again :D Here the path
should be renamed to helpPath
or helpContentPath
or something similar. path
is just too general
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.
helpPath: path
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.
@Pe5h4 ok, i renamed it
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.
@aringocode Alright, good
Changes
I tested it:
https://github.com/TeskaLabs/asab-webui/assets/65866093/9a4ac84f-d488-4fe9-8302-bf788eb2abfb