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

Context Selector #1857

Merged
merged 2 commits into from
Jul 10, 2020
Merged

Context Selector #1857

merged 2 commits into from
Jul 10, 2020

Conversation

andresmgot
Copy link
Contributor

Description of the change

Context Selector for Clarity UI. It allows to select the cluster and the namespace used.

Peek 2020-07-08 19-33

Applicable issues

Additional information

Note that the cluster selector is still not working. That's a WIP in a different branch.

Copy link
Member

@Angelmmiguel Angelmmiguel left a comment

Choose a reason for hiding this comment

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

LGTM in general. There are some comments before I approve this PR :)

const currentCluster = clusters.clusters[clusters.currentCluster];
const namespaceSelected = currentCluster.currentNamespace || defaultNamespace;
const [cluster, setCluster] = useState(clusters.currentCluster);
const [namespace, setNS] = useState(namespaceSelected);
Copy link
Member

Choose a reason for hiding this comment

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

I'd be consistent with the naming:

Suggested change
const [namespace, setNS] = useState(namespaceSelected);
const [namespace, setNamespace] = useState(namespaceSelected);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

setNamespace is already used (by a function of the props)

className={`dropdown kubeapps-align-center kubeapps-dropdown ${open ? "open" : ""}`}
ref={ref}
>
<button className="kubeapps-nav-link" onClick={toggleOpen} aria-expanded={open}>
Copy link
Member

Choose a reason for hiding this comment

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

Glad to see aria-expanded here :). I'm missing here the other aria property to link this button with the content: aria-haspopup. This property must use the same value as the role of the element with the content that it's placed next to the button:

Suggested change
<button className="kubeapps-nav-link" onClick={toggleOpen} aria-expanded={open}>
<button className="kubeapps-nav-link" onClick={toggleOpen} aria-expanded={open} aria-haspopup="menu">

Comment on lines 71 to 86
<Column span={10}>
<span>Current Context</span>
<div>
<CdsIcon size="sm" shape="cluster" inverse={true} />
<span className="kubeapps-dropdown-text">{clusters.currentCluster}</span>
<CdsIcon size="sm" shape="file-group" inverse={true} />
<span className="kubeapps-dropdown-text">{namespaceSelected}</span>
</div>
</Column>
<Column span={2}>
<div
className={`kubeapps-align-center angle ${open ? "angle-opened" : "angle-closed"}`}
>
<CdsIcon shape="angle" inverse={true} />
</div>
</Column>
Copy link
Member

Choose a reason for hiding this comment

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

Based on the design, I'm missing here some alignment and spacing between the items:

Design for the selector where all the items are aligned and they have more spacing

Also, any of the items is highlighted. I'd use a slightly bolder font for the selected items and a bit smaller font for the title.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated:
Screenshot from 2020-07-09 17-05-17

</span>
<div className="dropdown-menu-padding" role="menuitem">
<CdsIcon size="sm" shape="cluster" inverse={true} />
<span className="kubeapps-dropdown-text">Cluster</span>
Copy link
Member

Choose a reason for hiding this comment

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

Set this span as a label and link it with the select via htmlFor

</div>
<div className="dropdown-menu-padding" role="menuitem">
<CdsIcon size="sm" shape="file-group" inverse={true} />
<span className="kubeapps-dropdown-text">Namespace</span>
Copy link
Member

Choose a reason for hiding this comment

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

Same about the label.

Comment on lines 4 to 11
&-closed {
transform: rotate(180deg);
margin-left: -1rem;
}
&-opened {
position: relative;
left: 10px;
}
Copy link
Member

Choose a reason for hiding this comment

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

Did you try to use the rotate property of the icon component?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

mmh, I tried but there was no rotate property, turns out it's called direction

Copy link
Member

Choose a reason for hiding this comment

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

lol 😂

Comment on lines +97 to +105
<select name="clusters" className="clr-page-size-select" onChange={selectCluster}>
{Object.keys(clusters.clusters).map(c => {
return (
<option key={`kubeapps-dropdown-cluster-${c}`} value={c}>
{c}
</option>
);
})}
</select>
Copy link
Member

Choose a reason for hiding this comment

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

The style of the selector options is not the correct one. Clarity lefts the browser to apply their own style on those selectors:

Screenshot on how Clarity left selectors to use their native style based on the browser

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 don't understand what you are proposing, maybe it's a confusion? Note that I'm in linux so the style here is different:
Screenshot from 2020-07-09 17-20-37

Copy link
Member

Choose a reason for hiding this comment

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

Ohh I see! Completely forgot that

Copy link
Member

@Angelmmiguel Angelmmiguel left a comment

Choose a reason for hiding this comment

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

LGTM!

@andresmgot andresmgot merged commit bbf54c9 into master Jul 10, 2020
@andresmgot andresmgot deleted the contextSelector branch September 8, 2020 09:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants