-
-
Notifications
You must be signed in to change notification settings - Fork 3k
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
Small update to the UI's look and feel #395
Conversation
@rafaelconde so fresh and so clean! Really happy to see this PR! Things I love:
I really only have one high level concern: contrast. We need it. Your mocks in #179 use gray for the header and sidebar, what do you think of pulling that into this change? Same for the form fields - at least for this iteration, we should have distinct borders for them. I think we can pull it off without hurting the look. I mean you, you can pull it off. lol. |
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.
A few basic code review points here, nothing major.
@@ -2,6 +2,8 @@ | |||
|
|||
.appBar { | |||
background-color: var(--backgroundAltColor); | |||
height: auto; | |||
padding: 8px 2.4rem; |
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'd like to see us stick to pixels for units, but open to discussion on it if you feel strongly.
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'd love to see us move away from pixels across the board (better for accessibility), but that's a much longer discussion, and one I'll probably lose. :)
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'm with @verythorough on using relative units instead of pixels. Any reason you prefer pixels, @erquhart?
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.
@imorente the reasons for using px
are consistency with existing CSS and the fact that rem
s and em
s can cause half-pixel boundaries, which are undesirable. I have mixed feelings - those are both significant issues, but the accessibility problem is a hard one to ignore. For now, I think we should stick with px
for the simple reason that we shouldn't make a transition to relative units ad-hoc, even if we do end up using them at some point in the future.
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.
Yep, the idea is:
- Use pixels everywhere for now - simple and declarative.
- When we get an actual design system in place and get rid of all the hodge podge, bring in the ems and rems. It's more complex, but the benefits are there if we implement properly.
@@ -56,7 +56,6 @@ export default class EntryListing extends React.Component { | |||
<p>{entry.getIn(['data', inferedFields.descriptionField])}</p> | |||
: inferedFields.remainingFields && inferedFields.remainingFields.map(f => ( | |||
<p key={f.get('name')} className={styles.cardList}> | |||
<span className={styles.cardListLabel}>{f.get('label')}:</span>{' '} |
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 agree that this is redundant when viewing all entries of a given type, but it's necessary for the editorial workflow and search, where multiple entry types are present at once. If we're removing this we should add it back in for those views.
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'll leave this to you, I'll revert it back 👍
src/components/UI/card/Card.css
Outdated
@@ -3,31 +3,35 @@ | |||
.card { | |||
composes: base container rounded depth; | |||
overflow: hidden; | |||
} | |||
|
|||
.card > *:not(iframe, video, img, header, footer) { |
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.
At a glance I'm uncertain why these ungainly selectors were added - are you sure we don't need them?
src/components/UI/card/Card.css
Outdated
} | ||
|
||
.card > iframe, | ||
.card > video, | ||
.card > img { | ||
max-width: 100%; | ||
margin: -16px -24px 16px -24px; |
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.
Nitpick: you can drop the fourth value.
I can't wait for the facelift. My notes:
|
Comparing this: With the current editing screen: I think the current version is much easier to use. We need to make the UI look better, but it should never be at the cost of clarity and familiarity and I would bet good money that the current version will perform much, much better in any user testing with content editors that have never tried the CMS before. I think the steps @erquhart has been taking in the current version to move away from the more discrete UI elements I had in my initial mockups, and towards something that looks way more like traditional form elements from a Wordpress admin UI or the like, is the right approach. The #1 goal should be to make it really, really easy to see what is the preview, what is a form element, where you can click, where the different fields are separated, etc. |
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 really like the direction you've gone. I have a couple small nitpicks on the CSS changes, but those are trivial.
Some thoughts on the design changes:
-
While I think most of the entry draft fields such as single lines or images are pretty clear, the body is a bit confusing. It's not really clear where the textbox starts or ends, and the toolbar is a bit confusing since it looks almost identical to the contents of the body editor. It actually looks a little like I'm supposed to edit the text in the toolbar, since it has the same underline as the text fields above.
-
When the cards don't distribute evenly across the rows, extra cards at the bottom are stretched dramatically to fill the space. Is this intended?
-
The search box and the rest of the header have very similar background colors, which is kinda hard on my eyes. IDK if that's a real issue or just me.
margin-left: 15px; | ||
margin-bottom: 16px; | ||
margin-left: 16px; | ||
/*max-width: 31%;*/ |
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.
This should be either uncommented or removed. We shouldn't have commented-out code committed to the repo.
src/components/UI/card/Card.css
Outdated
letter-spacing: 0; | ||
line-height: 24px; | ||
padding: 0; | ||
color: #8c8c8c; |
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 this should be hardcoded - we should add it as a text color in theme.css
.
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.
LGTM
Needs rebase to ensure scrolling issues are still fixed.
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.
LGTM - this merges easily into @erquhart's upcoming text editor scrolling PR, and that fix still works correctly once that's done.
a65173f
to
9745ac2
Compare
Rebased and added a commit to address the following:
@rafaelconde let me know if you have thoughts on that last commit. I do think depth in the cards UI would be awesome, as well as a more elegant card grid presentation, but I think we need more polish when taking those steps. As it was, they were detracting from your stellar improvements. |
@biilmann I was thinking we could settle a bit on this PR, and fix the remaining contrast issues in a subsequent PR. Widgets and the editor footer are both on my radar. |
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.
Hold off on merging, fixing the sticky toolbar appearance.
@rafaelconde if you can improve the contrast issues that David and Matt pointed out, we should be good to merge. |
I addressed the contrast of popover and borders.
I also think that this could and should be addressed on a separate PR 🙏 |
@rafaelconde thanks, and yeah there's plenty to improve, and there will be more opportunity to do so. Just want to get this thing merged right now! @biilmann @calavera I'm good with Rafa's changes, and I'm planning to merge this at EOD if there are no objections. |
My main issue is that I can barely see the border in the image widget. That has not been addressed. |
Assuming @calavera is talking about this — I literally don't know how to change the border on that. |
@rafaelconde, I found the styles for the image widget 🎉 They're here: https://github.com/netlify/netlify-cms/blob/visual-tweaks/src/components/Widgets/ImageControl.js#L113 |
Thanks @imorente ! |
@calavera good? |
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.
@rafa just mentioned something I think we all forgot - the editorial workflow. He's planning to update for that soon, adding this as a requested change so we don't merge.
yes |
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.
Checked out the list and object widgets too, looks fine.
LGTM
- Summary
This PR offers a very minor, pure visual update to the UI.
Take it as quick and dirty fixes to how it looks, and hopefully the plan is to fan this spark into a flame, and consider a more thoughtful effort to rethink the User Interface.
- Test plan
- Description for the changelog
Updates typography, colors, and some UI visual tweaks
- A picture of a cute animal (not mandatory but encouraged)