UI related fixes here and there, towards using more PF4 #178
Conversation
* Fix inconsistent padding in header * Fix issue with line appearing above the list * Set the max width on the data list itself not the items, otherwise we see the borders spanning to the whole page Fixes cockpit-project#177
Footer is left aligned by default, no need to specify it.
KKoukiou
force-pushed
the
issue-177
branch
3 times, most recently
from
72892ce
to
af13dcb
Compare
They're not consistent with the rest of Cockpit they're not standard with PatternFly, and they're not using generally accepted spacing rules with respect to design most everywhere. Generally, when it comes to text, you want a certain amount of space above it and at the sides. Usually, there's more at the sides than the top. (GitHub and pretty much everything else does this too. Look at the buttons, for example.) It's because we read text horizontally and have spaces between words. And you want a certain amount of space between icons and text. It's usually around 0.5rem / 1rem. But it should be consistent. The picture is obviously not consistent (and there's wayyyyyy too much space between the (!) and the text it is supposed to direct attention to). The problem here is that it's an icon and then a link spaced as if they were separate items, instead of the same item. The icon + text in the second row is spaced together as if they belong together (and they do). Perhaps the top one has an icon and then a link instead of an icon in a link.
This one is not using the correct widgets or patterns we use elsewhere. There's a link with an icon that does an action. That should clearly be a button. The list has weird padding and spacing and doesn't look like other lists. We don't do editing like this anywhere else in Cockpit (or in PatternFly).
A lot of the issues would hopefully be solved if we used the correct patterns and widgets, and used them consistently. Anyway, I'm checking out the branch now to take a look at the changes. 😉 |
I think we need a redesign if we want to properly fix this. There's just too much going on here for a dialog; we probably need to make most of this a sub-page or a page section instead. However, redesigning this means we need to redesign the page itself so that it fits in properly too. Here's a first start at that:
I'm not very happy with the repository part yet, but this is better than what we have. There's probably a better set of widgets for it. Perhaps we really want to make it show only the current repository and make the editing behind some sort of disclosure widget or inline editing. I don't know. But the goal is to split apart the current repo dialog, so editing and adding can be their own separate dialogs, while the main part is not in a modal. We could probably drop architecture from being visible and filter the branch to be the only arch (or arches if there is more than one) avaialble. But we don't have to worry about the i386/x86_64 split. Would we need to worry about an ARM HFP / AArch64 split though? As for the … menus, the repository actions would be:
And the deployment actions would be:
(In addition to the main action of update + reboot / roll back + reboot.) We currently do not have pinning or unpinning capability in cockpit-ostree. This is how I think it would work, but it can be ignored and implemented later. |
There was a problem hiding this comment.
As far as this PR is concerned:
- 👍 The padding and spacing of the list has been much improved
- 👍 I'm happy the
<hr>line is gone - 👎 I'm not happy about using the table selection to indicate something is active... this was intended for selections to do something with
If you want to merge most of these changes in as-is without any big overhauls, I would be happy if we could rescope this PR to 1 & 2.
I think the dialog is a lost cause at this moment and we'd need to overhaul it to be like my mockup in the previous comment. But that might take time. So another PR would make sense for the larger changes.
|
I've copied the bulk of my redesign comment over on issue #176. |
|
@garrett agreed here - I removed the commits related to the dialog change |
|
The text in the heading doesn't line up to anything. It should either line up with the card's border, or the text of the card. (It's debatable which, but it doesn't line up with either now.)
The text of the card is inconsistent. Should the tabs line up with the heading and body text or should the other text line up with the tabs? (Same question, just which is anchored and which should be adjusted.)
Spacing of the headings is too large overall. But the available one where rolling back is possible is extremely odd and misaligned.
Additionally, there's "Title Capitalization" used all over the place and we're supposed to use "Sentence caps" in PF4, but this possibly belongs to another PR. |
|
We'll want to redo the page for #176, but it may take a bit longer, so we could still fix the obvious brokenness above meanwhile. |
… alignment of the title Normally this is autohandled by PF but we use hasNoPadding in order to add let the line bellow the navigation to expand to the left border of the card.
Fixed, it was because of the toolbar padding.
Fixed, adding some margin for it.
Let's not address it in this commit. The paddings used are exactly the same as the documentation's https://www.patternfly.org/v4/components/data-list#actions-single-and-multiple If you think these are wrong, as always let's open an issue upstream.
Right, another pR :) |
KKoukiou
dismissed
garrett’s stale review
Please rereview- addressed the issue which are related to the initial issues I opened this PR fore.
|
I'm getting errors when loading the page. Oops appears and the console has a LOT of spew. I think it's because remotesList is null, yet attempts <Button key="change-repo"
variant="primary"
isDisabled={!!editRepoDialogOpen || !remotesList.includes(selectedRemote)}
onClick={() => {
onChangeRemoteOrigin(selectedRemote).then(() => setIsModalOpen(false), ex => setError(ex.message));
}}>Here's the log (sorry; it's a screenshot... not really a good way to copy/paste it all): |
|
@garrett unrelated to this PR the errors you see above. |
Really? I cannot run this PR due to those errors. Are they in master too then? (Going to build master now) |
|
After switching to master, building, and seeing it work, I switched back to this branch and built it again and it worked. It's really odd, as I did a make clean after the first time too. 🤷 |
There was a problem hiding this comment.
Spacing of the headings is too large overall. But the available one where rolling back is possible is extremely odd and misaligned.
Let's not address it in this commit. The paddings used are exactly the same as the documentation's https://www.patternfly.org/v4/components/data-list#actions-single-and-multiple
But that's not the widget that should be used here? That's supposed to be a card title, not a list, right? 🤔
Everything other than the heading is correct now. There are enough huge improvements that we should just get this in and work on a follow-up to properly fix the heading.
For reference, the heading still has the alignment and spacing issues:
- Orange: Space above the button (also reflected below). It's the same. That's vertically centered.
- Purple: Space above the text (also reflected below). It's not the same. It's 5px larger below (shown in dark red).
- Lighter red: offset from baseline: 6px
- Darker red: difference between spacing above and below the text: 5px
Note: I'm not suggesting shifting things by pixels in overrides or anything like that. Just showing that they're off and not properly aligned, and by how much.
But everything else (aside from the heading, mentioned above, and the dialog, mentioned in the linked issue) is good now. Thanks!
Approving for the fixes this is addressing.
No description provided.