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.

Sign up for GitHub

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

First pass at sub-bar redesign. #1245

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
+82 −56
Open

First pass at sub-bar redesign. #1245

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions src/components/EditorContainer.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,12 +21,12 @@ function EditorContainer({children, language, source, style, onHide, onRef}) {
style={prefixAll(style)}
>
<div
className="editors__label editors__label_expanded"
className="editors__label editors__label_expanded sub-bar"
onClick={onHide}
>
<span className="u__icon editors__chevron">&#xf078;</span>
{t(`languages.${language}`)}
{' '}
<span className="u__icon">&#xf078;</span>
</div>
{helpText}
{children}
Expand Down
4 changes: 2 additions & 2 deletions src/components/EditorsColumn.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -114,10 +114,10 @@ export default class EditorsColumn extends React.Component {
`editor.${language}`,
)}
>
<div className="editors__label editors__label_collapsed">
<div className="editors__label editors__label_collapsed sub-bar">
<span className="u__icon editors__chevron">&#xf077;</span>
{t(`languages.${language}`)}
{' '}
<span className="u__icon">&#xf077;</span>
</div>
</div>
));
Expand Down
24 changes: 14 additions & 10 deletions src/components/Preview.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,16 +28,20 @@ export default function Preview({

return (
<div className="preview output__item">
<div className="preview__title-bar">
<span
className="preview__button preview__button_pop-out u__icon"
onClick={onPopOutProject}
>&#xf08e;</span>
{title}
<span
className="preview__button preview__button_reset u__icon"
onClick={onRefreshClick}
>&#xf021;</span>
<div className="preview__title-bar sub-bar">
<div className="sub-bar__left">
{title}
</div>
<div className="sub-bar__right">
<span
className="sub-bar__button"
onClick={onPopOutProject}
>Full screen</span>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I find the icons more usable here (the eye finds what it’s looking for more quickly if it doesn’t have to read the words). And I have never noticed students finding them unintuitive. Have you?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just looking for ways to increase overall clarity and discoverability. Anecdotally I see a lot of students don't know that those icons are even actions at all, let alone what they do. Would you be averse to icon + text?

<span
className="sub-bar__button"
onClick={onRefreshClick}
>Refresh</span>
</div>
</div>
{projectFrames}
</div>
Expand Down
106 changes: 64 additions & 42 deletions src/css/application.css
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,7 @@
:root {
--color-low-contrast-gray: #eee;
--color-chrome: #e2e2e2;
--color-border: #bbb;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we really need another gray tone?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd say yes. I tried the existing ones but they looked ugly and too pronounced.

--color-light-gray: #aaa;
--color-gray: #666;
--color-dark-gray: #444;
Expand All @@ -82,6 +83,7 @@
--color-blue: #00b8ff;
--size-rounded-corners: 0.2em;
--size-top-bar: 4em;
--size-sub-bar: 2em;
--size-top-bar-menu-button: 2.8em;
--box-shadow: 0 0 0.1em 0 rgba(0, 0, 0, 0.5);
--font-size-menu: 1.7vh;
Expand Down Expand Up @@ -168,6 +170,7 @@ body {

.top-bar__wordmark-container > svg {
width: 6.5em;
height: var(--size-top-bar);
}

.top-bar__snapshot_in-progress {
Expand Down Expand Up @@ -292,6 +295,57 @@ body {
flex: 1 0 0;
}

/** @define sub-bars */

.sub-bar {
box-shadow: var(--box-shadow);
font-size: var(--font-size-menu);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we need a new name for this font size variable!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps. It's becoming a general goto "base" font-size.

font-family: "Roboto";
padding-left: 0.5em;
height: 2em;
display: flex;
align-items: center;
text-align: left;
background-color: var(--color-chrome);
z-index: 2;
}

.sub-bar__left,
.sub-bar__right {
white-space: nowrap;
}

.sub-bar__left {
overflow: hidden;
text-overflow: ellipsis;
padding-right: 0.5em;
flex: 1;
}

.sub-bar__right {
text-align: right;
}

.sub-bar__button {
border-left: 0.1em solid var(--color-border);
padding: 0 1em;
height: var(--size-sub-bar);
line-height: var(--size-sub-bar);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we not use percentages here?

display: inline-block;
color: var(--color-gray);
font-weight: bold;
cursor: pointer;
user-select: none;
}

.sub-bar__button:hover {
background-color: var(--color-low-contrast-gray);
}

.sub-bar__button:active {
box-shadow: inset 0 0.1em 0.2em 0 rgba(0, 0, 0, 0.3);
}

/** @define instructions */

.instructions {
Expand Down Expand Up @@ -390,23 +444,11 @@ body {
}

.editors__label {
background-color: var(--color-chrome);
color: var(--color-dark-gray);
cursor: pointer;
font-family: 'Roboto';
font-size: var(--font-size-menu);
padding: 0.5em 1em;
text-align: right;
user-select: none;
}

.editors__label_expanded {
background-color: rgba(226, 226, 226, 0.9);
box-shadow: var(--box-shadow);
position: absolute;
right: 0;
top: 0;
z-index: 1;
.editors__chevron {
margin-right: 0.5em;
}

.editors__column-divider {
Expand Down Expand Up @@ -473,7 +515,7 @@ body {
left: 0;
position: absolute;
right: 0;
top: 4.4vh;
top: 3.4vh;
z-index: 0;
}

Expand All @@ -490,35 +532,10 @@ body {
}

.preview__title-bar {
box-shadow: var(--box-shadow);
font-size: 2.2vh;
height: 2.4vh;
position: absolute;
top: 0;
left: 0;
right: 0;
padding: 1vh;
position: absolute;
text-align: center;
font-weight: bold;
vertical-align: middle;
background-color: var(--color-chrome);
z-index: 2;
}

.preview__title {
text-align: center;
}

.preview__button {
padding: 0 0.5em;
}

.preview__button_pop-out {
float: left;
}

.preview__button_reset {
float: right;
}

/** @define error-list */
Expand Down Expand Up @@ -566,11 +583,16 @@ body {
/** @define notification-list */

.notification-list__notification {
margin: 0.2rem 0;
font-size: var(--font-size-menu);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems unrelated to the other stuff in here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Somewhat. Kinda rolling it into general "chrome/menu/etc." design tweaks to reign in the sizing and spacing of everything and make things look more standard. Would you be totally averse to keeping it in here?

margin-bottom: 0.1em;
padding: 1rem 0;
text-align: center;
}

.notification-list__notification:last-child {
margin-bottom: 0;
}

/* postcss-bem-linter: ignore */
.notification-list__notification a {
color: inherit;
Expand Down