-
Notifications
You must be signed in to change notification settings - Fork 32
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
WIP - Add ability to generate report without uploading #849
Conversation
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.
Thanks @chris1984, hope my review is readable, I gave you some other option if you want but since you already invested quite a lot in this PR, I also suggested a small fix to the current issue
<Dropdown | ||
aria-label="restart_dropdown" | ||
ouiaId="restart_dropdown" | ||
toggle={ | ||
<DropdownToggle | ||
aria-label="restart_report_toggle" | ||
ouiaId="restart_report_toggle" | ||
splitButtonItems={[ | ||
<DropdownToggleAction key="action" aria-label="bulk_actions" onClick={onRestart}> | ||
{__('Restart')} | ||
</DropdownToggleAction>, | ||
]} | ||
splitButtonVariant="action" | ||
toggleVariant="primary" | ||
onToggle={onActionToggle} | ||
isDisabled={isExitCodeLoading(exitCode)} | ||
/> | ||
} | ||
isOpen={isActionOpen} | ||
dropdownItems={dropdownItems} | ||
/> |
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 have already done a lot of work to fit this in, but if interested this could have been implemented in a separate component with "newer" technologies even without using redux actions reducers and all that stuff.
for the API call you could use useAPI
import { useAPI } from 'foremanReact/common/hooks/API/APIHooks';
import { noop } from 'foremanReact/common/helpers'; | ||
import { sprintf, translate as __ } from 'foremanReact/common/I18n'; | ||
import { isExitCodeLoading } from '../../ForemanInventoryHelpers'; | ||
import './tabHeader.scss'; | ||
|
||
const TabHeader = ({ exitCode, onRestart, onDownload, toggleFullScreen }) => ( | ||
const onActionToggle = () => { | ||
setIsActionOpen(prev => !prev); |
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.
Where is this setter coming from?
I think it would be better to use React's useState
inside the TabHeader
component to trigger a component re-render
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 copied and pasted that in from the Katello Errata tab, since we needed an onActionToggle
That was probably a bad idea, but I am guessing we are missing something else.
ouiaId="restart_disconnected" | ||
key="restart_disconnected" | ||
component="button" | ||
onClick={restartDisconnected} |
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.
in case you would still like to use Redux in this PR,
the prop restartDisconnected
would be passed to the TabHeader
component, maybe you want to pass the dropdownItems
into that component, e.g:
const TabHeader = ({ exitCode, onRestart, onDownload, restartDisconnected, toggleFullScreen }) => {
const dropdownItems = [....]
return <Grid.Row className="tab-header">.......</Grid.Row>
}
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.
When I put in const dropdownItems = [....]
everything lights up red like a stop light 🤣 the error from vscode says
Identifier expected. 'const' is a reserved word that cannot be used here.ts(1359)
looks like you need either to update the package.json with `"cosmiconfig-typescript-loader": "^4.0.0", |
Blah that didn't work, I remember we had to pin that to 4.0.0 a while back because the tests started to fail and we got a dependency error. @ShimShtein can probably tell more about it |
Since we are close to the dev freeze, i'm going to close this one out and just open a PR for the backend change and then expose that api to the rails controller so hammer can use it. We can add the UI button at a later time. |
Need to figure out why i'm getting
And also update the JS snapshots so tests will pass.
@ShimShtein let me know if the backend change looks fine, I sent a link to this pr to @Ron-Lavi so he can help with the JS stuff
Looks like the JS tests GitHub action is failing to even start with our favorite thing