Fixes #29298 - Add new Content View table #8644
Conversation
|
Issues: #29298 |
| compoundParent: 5, | ||
| cells: [ | ||
| { | ||
| title: <VersionsExpansion versions={versions} cvId={id} />, |
There was a problem hiding this comment.
All of these expansion panes are being passed the appropriate info, but the components aren't actually using it yet. That will come in future PRs 👍
|
This page uses dummy data, the redux portion is coming in another PR or will be added here later. Feel free to alter the dummy data in Since this is under "labs", I kept the dummy data checked into git and many pieces are not hooked up yet, such as the actions and the expandables are empty. "Details" will be a link to a details modal with more information than what is shown in the expandable panes. There is no link in this PR now. The expandables will load the details data for the CV, this will be through redux making the API call once that part is ready, for now it is dummy data. It will also only make the details call once, so clicking on the other expandables in that row or closing and re-opening the expandable won't keep calling the API. CV page list CV page with some expandables open CV page with no CVs present |
|
A note on the API call: |
|
Lots of comments by me! 😃 Just want to give a brain dump before I forget my various trains of thought during development. Feel free to make use of the "hide comments" option on files 😉 |
|
There is some question of what parts of the pf4 table components should be moved to shared components, but its a bit early to tell right now. I think a second page using a pf4 table would give a better idea on how to break it up. But for now, I'll try to keep things as modular as possible and keep in mind that parts may abstracted out. 👍 |
|
To test this, check out the PR, change the following in Foreman, and diff --git a/package.json b/package.json
index a3db80a..4795653 100644
--- a/package.json
+++ b/package.json
@@ -20,7 +20,7 @@
"create-react-component": "yo react-domain"
},
"dependencies": {
- "@theforeman/vendor": "^4.0.7",
+ "@theforeman/vendor": "^4.3.0",
"intl": "~1.2.5",
"jed": "^1.1.1",
"react-intl": "^2.8.0"
@@ -30,11 +30,11 @@
},
"devDependencies": {
"@babel/core": "^7.7.0",
- "@theforeman/builder": "^4.0.7",
- "@theforeman/eslint-plugin-foreman": "^4.0.7",
- "@theforeman/stories": "^4.0.7",
- "@theforeman/test": "^4.0.7",
- "@theforeman/vendor-dev": "^4.0.7",
+ "@theforeman/builder": "^4.3.0",
+ "@theforeman/eslint-plugin-foreman": "^4.3.0",
+ "@theforeman/stories": "^4.3.0",
+ "@theforeman/test": "^4.3.0",
+ "@theforeman/vendor-dev": "^4.3.0",
"argv-parse": "^1.0.1",
"babel-eslint": "^10.0.0",
"babel-loader": "^8.0.0", |
There was a problem hiding this comment.
There are a lot of missing translations here that should be translated with __(). I commented on some of them but didn't get them all.
Also I am seeing these compilation errors:
20:38:29 webpack.1 | ERROR in ../katello/node_modules/@patternfly/react-styles/css/assets/images/pfbg_992@2x.jpg
20:38:29 webpack.1 | Module parse failed: Unexpected character '�' (1:0)
20:38:29 webpack.1 | You may need an appropriate loader to handle this file type.
20:38:29 webpack.1 | (Source code omitted for this binary file)
20:38:29 webpack.1 | @ ./node_modules/css-loader!../katello/node_modules/@patternfly/react-styles/css/components/AboutModalBox/about-modal-box.css 6:5304-5350
Seems like we don't have a webpack loader for images?
johnpmitsch
force-pushed
the
new-cv-table
branch
2 times, most recently
from
7797a8e
to
31be6d8
Compare
There was a problem hiding this comment.
Thanks for bringing PF4 into existence in Katello, @johnpmitsch ! Lots of small comments below, but overall I think my main issues are not with the code, but I'm just coming out of this less excited for PF4 than I was before. It appears the same PF3 paradigm applies in PF4 for tables, with lots of arrays and objects being passed in to Patternfly. This is totally in conflict with the composable nature of JSX and React in general, so it seems to me like we're going to end up with a React code base just as complex and difficult as it was with PF3. On the bright side, the visuals and styling do look much better and more modern.
I didn't run the new testing library, so no testing comments for now. But I have played around with react-testing-library before and I really like the philosophy.
One overall visual comment-- it seems like the scale is a bit off. The font sizes and everything look huge, and too bold. It makes the checkboxes and the nav bar menus look tiny. Are these sizes the default?
webpack/scenes/ContentViews/expansions/RepositoriesExpansion.js
Outdated
Show resolved
Hide resolved
johnpmitsch
force-pushed
the
new-cv-table
branch
3 times, most recently
from
dce0650
to
5ce8ee1
Compare
| const ContentViewTable = ({ | ||
| loadContentViewDetails, detailsMap, results, loading, | ||
| }) => { | ||
| const [table, setTable] = useState({ rows: [], columns: [] }); |
There was a problem hiding this comment.
@johnpmitsch (Documenting/expanding our IRC discussion here)
I think that since expandedColumnMap only stores data about what rows are expanded, the right place for this is in internal component state.
For the table rows and columns, I can see the argument for moving to Redux. The task is to take the raw API response and format it in the way that PF4 requires. The question is where should this "data normalization" be done?
There are a few possible approaches:
- Use the
react-reduxhookuseSelectorto read the API response. Format the data in the component usingtableDataGeneratorlike you're already doing. (May require some refactoring oftableDataGenerator, but otherwise not too much change - Write a custom hook,
useForemanTableData, that does the same thing as the above. This way it could be reused across several tables. - Have the Redux reducer format the API response and just store the pre-formatted data in Redux. Then you could use
useSelectorin the component but not worry about data formatting there. - Write a new Redux middleware that looks for the Content View SUCCESS action, formats the data, and dispatches a new action which stores the formatted data in Redux. This keeps business logic out of the reducer, which really should be only for transforming actions into new state.
Any of these will avoid using useState for the table data. I'd be perfectly happy with #1 for now, and either #2 or #4 eventually.
| const ContentViewTable = ({ | ||
| loadContentViewDetails, detailsMap, results, loading, | ||
| }) => { | ||
| const [table, setTable] = useState({ rows: [], columns: [] }); |
There was a problem hiding this comment.
Which makes me think, @LaViro is there a way for plugins to add Redux middleware to Foreman? I think we'd probably have to build that.
| onClick: (_event, rowId, rowInfo) => console.log(`clicked on row ${rowId} with Content View ${cvIdFromRow(rowInfo)}`), | ||
| }, | ||
| ]; | ||
| /* eslint-enable no-console */ |
There was a problem hiding this comment.
I agree these should eventually be in Redux but it's not needed this early
| return ( | ||
| <Table | ||
| aria-label="Content View Table" | ||
| onSelect={cvsPresent ? onSelect : null} |
There was a problem hiding this comment.
Why pass null here? Isn't it always expecting a function anyway?
There was a problem hiding this comment.
Before I refactored, the empty state view when you don't have any rows will display a checkbox if the function is passed in to this parameter. But now that its not displaying the table in that state, this can probably be removed. I'll try it 👍
| const row = [ | ||
| { title: <ContentViewName composite={composite ? 1 : undefined} name={name} cvId={id} /> }, | ||
| lastPublished || 'Not yet published', | ||
| { title: __('Details'), props: { isOpen: false, ariaControls: `cv-details-expansion-${id}`, contentviewid: id } }, |
There was a problem hiding this comment.
After thinking a bit more over lunch, maybe it's better to handle the data this way:
Your formatted data will be not just the array of rows, but an array of objects like
{
isExpanded: false,
contentViewId: 1,
contentViewDetails: {..},
rowData: [...],
}That way you have all the data where you need it, attached to the row data. And then you can map THAT to get your rows prop to pass down.
(This will also eliminate the confusing data shape in the expanded rows map.)
There was a problem hiding this comment.
(This does maybe put the isExpanded state in Redux where it doesn't need to be. But maybe you could map that separately in the component state.)
There was a problem hiding this comment.
Thanks, let me play around with it. I'm leaning towards some sort of row mapping as well, but not sure how much should live in redux and how much should be in the component. I'll see what feels right!
johnpmitsch
force-pushed
the
new-cv-table
branch
5 times, most recently
from
91fd1d9
to
62a0f4c
Compare
|
@LaViro @jeremylenz I updated to use the API middleware, along with some other updates based on comments. I still have to update both the testing and the linting, and there is some general cleanup still to-do. I'm pushing up now if you want to give feedback on the general direction to make sure its what you expected. Please hold off on any detailed comments until I get everything cleaned up, but feel free to give comments on the general refactoring! 😉 To-do:
|
johnpmitsch
force-pushed
the
new-cv-table
branch
2 times, most recently
from
4df69dd
to
49b31ef
Compare
| "@theforeman/builder": "^4.3.0", | ||
| "@theforeman/vendor-dev": "^4.3.0", | ||
| "@theforeman/builder": "^4.2.0", | ||
| "@theforeman/vendor-dev": "^4.2.0", |
There was a problem hiding this comment.
I gotta change it back, bad git rebase! Really this should be 4.5.0 like theforeman/foreman#7519 - good catch
Thanks and agreed! Sorry, I should have commented back with an update, everything is fair game to review except the tests, which need to be updated, so have at it 😉 |
There was a problem hiding this comment.
Thanks @johnpmitsch
Getting there! Some code comments below
| Object.entries(rowMapping).forEach(([cvId, { rowIndex }]) => { | ||
| if (rowIndex === rowIdx) id = cvId; |
There was a problem hiding this comment.
Feel like it's cleaner to use find:
const entry = Object.entries(rowMapping).find(item => item[1].rowIndex === rowIdx);
if (entry) return parseInt(entry[0]);| const EmptyTitle = __("You currently don't have any Content Views."); | ||
| const EmptyBody = __('A Content View can be added by using the "New content view" button below.'); |
There was a problem hiding this comment.
String values should be normal camel case, not title case
|
|
||
| contentViews.forEach((contentView, rowIndex) => { | ||
| const { id } = contentView; | ||
| if (!Object.prototype.hasOwnProperty.call(updatedRowMapping, id)) { |
There was a problem hiding this comment.
I'm very confused.. why do you need call here? And why use Object.prototype ?
There was a problem hiding this comment.
eslint yelled at me! 😳 https://eslint.org/docs/rules/no-prototype-builtins
There was a problem hiding this comment.
Wow. I can see that it's a reasonable rule but it sure makes the code hard to read and understand. Thanks for explaining :)
There was a problem hiding this comment.
Actually I would like a comment pointing to that eslint rule.
| {title} | ||
| </Title> | ||
| <EmptyStateBody> | ||
| {body } |
| @@ -0,0 +1,3 @@ | |||
| .ktable-cell-icon { | |||
| margin: 0 8px 0 0; | |||
There was a problem hiding this comment.
I can never remember the order for these CSS shortcuts (top bottom left right? top right bottom left?) When it's just a single value, would you be open to just using margin-left or whatever it is?
| useEffect(() => { | ||
| if (isOpen && Object.keys(details).length === 0) { | ||
| dispatch(getContentViewDetails(cvId)); | ||
| } | ||
| }); |
There was a problem hiding this comment.
Without a dependency array, wouldn't this repeatedly make API requests on every render until the first one comes back?
| }); | ||
|
|
||
| if (status === STATUS.PENDING) return (<Loading size="sm" />); | ||
| // Can we display the error message? |
There was a problem hiding this comment.
I don't think this comment is necessary; the next line is pretty self-explanatory 😋
There was a problem hiding this comment.
that was actually for me to not forget, with this we don't show the actual API response, I can remove it for now though and file an issue
There was a problem hiding this comment.
Oh that's fine then, if it's for you to remember something then just stick a TODO in front of it.
There was a problem hiding this comment.
just stick a TODO in front of it.
I'm not a fan of these, mostly because they just stick around and nothing happens with them and we have a proper issue tracking system, so I'll just file an issue and associate with the tracker
There was a problem hiding this comment.
| const combinedReducers = combineReducers({ katello: combineReducers({ [namespace]: reducer }) }); | ||
| // Namespacing the initial state as well | ||
| const initialKatelloState = Immutable({ katello: { [namespace]: initialState } }); | ||
| const store = createStore(combinedReducers, initialKatelloState, applyMiddleware(thunk)); |
There was a problem hiding this comment.
Now that foremanReact can be used for testing, is this resolved now?
|
@jeremylenz This has been fully updated now, though the tests will still fail. There will need to be a Foreman PR to remove deprecations that is causing some sort of cyclical dependency and I also had to update the dev dependencies to include a new package to mock network calls. Both of these are semi-related, I'll explain: Problem and solution 1: mocking network requestsUsually packages are expected to be mocked by the application when they are directly used by the application itself. We have been doing this with axios-mock-adapter as we have been previously using axios by calling it directly in Katello. So in production this looks like: Katello -> axios -> nodejs -> http call and we can easily do the following with Katello -> mocked version of axios up until now, this has been working fine. But this PR is now using the API middleware from Katello -> foremanReact -> axios -> nodejs -> http call So this is now is a bit more complicated. If we use So basically we can't do this: Katello -> foremanReact -> mocked version of axios But we can use Katello -> foremanReact -> axios -> mocked http module of nodejs which works! So I used nock in this PR and it allows us to use the foremanReact API middleware as-is, but still mock the network calls. I usually try to avoid adding more dependencies, but this is a just a development dependency and the approach we have been using (which works because we only used axios in Katello) won't work with our architecture. So from this and react-testing-library, I was able to only mock the network requests and use the component fully connected. I moved all the testing to the top-level page and made sure I had the same tests as previously. The only one I am going to wait on are the expansion row tests, as there is not much in them yet to check for on screen. I would rather wait until they are built out more to write tests, otherwise I wind up doing things like adding test-id's And now for the other problem: Problem and solution 2:
|
|
@johnpmitsch Thanks for the explanation! Adding the |
|
@jeremylenz I opened the Foreman PR here theforeman/foreman#7638 - there is just one test left to figure out there. Please take another look and see if you have any other concerns for this PR |
There was a problem hiding this comment.
@johnpmitsch No further concerns, ACK pending successful test runs with the Foreman PR.
| 109, | ||
| 108, | ||
| 111, | ||
| 110 |
There was a problem hiding this comment.
I think using 2 contents repos and ids would be more than enough as a fixture,
And reduce the size of the snaps too
There was a problem hiding this comment.
There are no snaps since this is using r-t-lib, is there a more specific concern? These are actual API responses, the file is only 5kb and is just used in testing.
|
[test katello] |
This adds a new content view table using patternfly 4 to the page "/labs/content_views"
|
[test katello] |
This adds a new content view table using patternfly 4 to the page "/labs/content_views"