-
Notifications
You must be signed in to change notification settings - Fork 3.4k
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
Add Completed Job list tab to multiple resources #6058
Add Completed Job list tab to multiple resources #6058
Conversation
* Resources include: Host, InventoryHost, Inventory, SmartInventory, Template, and WFTemplate * Move JobList into top-level shared component directory
d536e30
to
37a33f9
Compare
Build succeeded.
|
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 think defaultParams
is fine - defaultSearchParams
if you wanted to be more verbose 🤷♂
const { | ||
data: { count, results }, | ||
} = await UnifiedJobsAPI.read(params); | ||
} = await UnifiedJobsAPI.read({ ...params, ...defaultParams }); |
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.
is it possible to have some of the same values in params
, and defaultParams
, and if so will that cause issues with the api?
</DataListCell>, | ||
<DataListCell key="name"> | ||
<span> | ||
<Link to={`/jobs/${JOB_TYPE_URL_SEGMENTS[job.type]}/${job.id}`}> |
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 think if we are going to use constants to help us build our urls for Links we need to be more consistent in doing that throughout the whole project. Just sharing thoughts.
Also, I think you and I talked about the idea that if we are linking to a url that ends in Id, just so it can be redirected to whatever/id/details, we should just link directly to details...or in this case to /output.
); | ||
expect(wrapper.find('DataListCell[aria-label="type"]').length).toBe(1); | ||
}); | ||
}); |
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.
Because you are using this component for so many different types of jobs it might make sense to write a test that asserts that when clicking on the job name it navigates to the correct url.
@@ -64,24 +64,25 @@ class Inventories extends Component { | |||
[`/inventories/${inventoryKind}/${inventory.id}/hosts/add`]: i18n._( | |||
t`Create New Host` | |||
), | |||
[`/inventories/${inventoryKind}/${inventory.id}/hosts/${nestedResource && | |||
nestedResource.id}`]: i18n._( |
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 don't think you need to mark these variables for translation...and you could use optional chaining here
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 didn't notice that they were marked for translation, I only moved its position up in the config. Next time I'm in this file I'll check it out.
Build succeeded (gate pipeline).
|
…lue|role] settings (ansible#12558) (ansible#6058)
SUMMARY
Issues: #5890 #5892 #5903 #6061
Template Completed Job List Mockup
Add Completed Jobs tab to:
I decided to move the JobList and JobListItem components to the shared component directory and use them in all of the tab job list views. For this to work in all of the above locations, each parent component passes a
defaultParams
prop with a query object to < JobList /> which then passes those params toUnifiedJobsAPI.read()
. This is exactly how we achieve it in the old UI. I'm open to changing the prop name if it is too generic.This PR also deletes the HostCompletedJobs, InventoryCompletedJobs, and SmartInventoryCompletedJobs directories; and refactors Jobs.jsx into a functional component.
Only the
/jobs
list shows a type column. The < Jobs /> component passes a boolean to propshowTypeColumn
and drills it down to the < JobListItem /> component which checks the value to show or hide the type DataListCell. It seemed like a simple solution, but I'm also open to other ideas.ISSUE TYPE
COMPONENT NAME
SCREENSHOTS
Smart Inventory List
Inventory Host List
Job List