Skip to content
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

Convert list views to table structure to fix alignment issues #1031

Merged
merged 11 commits into from Sep 19, 2019

Conversation

@mturley
Copy link
Contributor

mturley commented Sep 11, 2019

Closes #1025
Closes #993 (https://bugzilla.redhat.com/show_bug.cgi?id=1716845)

This PR introduces the new ListViewTable and ListViewTableRow components, which match the props interface for the pf3 ListView and ListViewItem, respectively. The ListViewTable converts the floating-divs HTML structure of the ListView into table HTML, to properly align its items as real columns.

Because this breaks responsiveness on mobile viewport sizes, it is only used at desktop window size, and smaller windows will fall back to the original ListView implementation (which is possible because of the matching props interface). The BreakJS-based layout listener (which I wouldn't have added, but is already used elsewhere in pf3) swaps the two render implementations out automatically on window resize.

This PR converts the Plans Not Started and Plans Complete/Archived list views to use the new structure. In separate PRs, I'll convert the Plan Details list view, and also convert the Plans In Progress card view to this kind of list view.

I also worked out some CSS quirks, and in general slimmed down some of the margin/padding in these list view items, in anticipation of the higher-density list view items we'll need for warm migration.

Screens

Before:

Screenshot 2019-09-11 17 06 45

After:

Screenshot 2019-09-11 17 05 50

@mturley mturley requested a review from mzazrivec Sep 11, 2019
@mturley mturley added this to In progress in v2v UI via automation Sep 11, 2019
@mzazrivec

This comment has been minimized.

Copy link
Contributor

mzazrivec commented Sep 12, 2019

The CI failures seem related.

@mturley mturley changed the title List view table structure Convert list views to table structure to fix alignment issues Sep 12, 2019
@mturley

This comment has been minimized.

Copy link
Contributor Author

mturley commented Sep 12, 2019

Yep, I need to fix the tests and apparently something about how the layout import behaves in the tests. We might want to wait to merge this until after #1030 anyway, I'll fix this after I finish reviewing that and fixing some issues I found.

@mturley

This comment has been minimized.

Copy link
Contributor Author

mturley commented Sep 16, 2019

Fixed the CI failures here, I needed to update enzyme so it could understand the Context API.

@mturley

This comment has been minimized.

Copy link
Contributor Author

mturley commented Sep 17, 2019

(edit: commented on the wrong PR, sorry)

@mturley mturley force-pushed the mturley:list-view-table-structure branch from 5a51e14 to bb1b157 Sep 17, 2019
</ListView.InfoItem>
) : null;

ScheduledTimeInfoItem.propTypes = {

This comment has been minimized.

Copy link
@codeclimate

codeclimate bot Sep 17, 2019

Similar blocks of code found in 7 locations. Consider refactoring.

@mturley

This comment has been minimized.

Copy link
Contributor Author

mturley commented Sep 17, 2019

I pushed another commit to abstract the scheduled time info item into a component shared by the not started list and the completed list, to satisfy codeclimate. But, codeclimate now has a problem with the propTypes being too similar to other propTypes lists, I guess? Except the lists don't even have the same props. I think this is invalid.

Reading up on codeclimate, it looks like it's possible to mark certain issues as invalid or wontfix (https://docs.codeclimate.com/docs/issues) which would be really useful for getting rid of that red X in these cases without totally ignoring codeclimate. @Fryguy @agrare do you guys know if that's something I can request permission for in codeclimate, and if so, who manages those permissions? Currently I don't have permission to set the issue statuses described in that doc.

@mturley mturley force-pushed the mturley:list-view-table-structure branch from bb1b157 to c3d2184 Sep 17, 2019
@mturley mturley added the bz label Sep 17, 2019
</tr>
);
const numColumns = row.props.children.filter(child => !!child).length;
console.log('TODO: use this for colspan of expanded content -- numColumns =', numColumns);

This comment has been minimized.

Copy link
@mzazrivec

mzazrivec Sep 18, 2019

Contributor

Do we need the console.log here?

This comment has been minimized.

Copy link
@mturley

mturley Sep 18, 2019

Author Contributor

Ah, good catch, I forgot about that. I was intending originally to implement expandable rows in this PR, but I decided to save that for another PR and forgot to remove this.

@mzazrivec

This comment has been minimized.

Copy link
Contributor

mzazrivec commented Sep 18, 2019

The changes look good to me and have no problem merging, except that one console.log (which may stay, will wait for Mike's opinion here).

@miq-bot

This comment has been minimized.

Copy link
Member

miq-bot commented Sep 18, 2019

Checked commits mturley/manageiq-v2v@6624ff1~...f9bb58c with ruby 2.4.6, rubocop 0.69.0, haml-lint 0.20.0, and yamllint 1.10.0
0 files checked, 0 offenses detected
Everything looks fine. 🍰

@mzazrivec mzazrivec self-assigned this Sep 19, 2019
@mzazrivec mzazrivec merged commit 40746f1 into ManageIQ:master Sep 19, 2019
2 of 3 checks passed
2 of 3 checks passed
codeclimate 1 issue to fix
Details
Hakiri No security warnings were found.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
v2v UI automation moved this from In progress to Done Sep 19, 2019
@mturley mturley deleted the mturley:list-view-table-structure branch Sep 19, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
v2v UI
  
Done
3 participants
You can’t perform that action at this time.