-
Notifications
You must be signed in to change notification settings - Fork 18
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
App Bar implementation #11
App Bar implementation #11
Conversation
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 from my very limited understanding of React 😅 Just a few small questions/comments again :)
client/src/components/Link/index.ts
Outdated
@@ -0,0 +1 @@ | |||
export * from './Link'; |
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 feel like making a whole "subpackage" (idk what you call it in js) for such a small component seems overkill. Maybe this belongs in MenuDrawer
, or, if it's more generic, in some utility package.
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.
It's a module. Modules can be directories if it has an index.ts
, or it can be its own file. We'll go over it our TypeScript workshop 😄
I don't think there's necessarily a limit to how large a component needs to be. As an example, here's probably the smallest component I could find within CZI: https://git.io/JOGXP
The only difference is that we have an index.ts
exporting the component and a unit test 😆 The reason I don't define components in the index.ts
file is because:
- I like thinking of the
index.ts
file as responsible for exporting exports from submodules. - Having a named file like
Link.tsx
is easier to work with thanindex.ts
. One example I can think of is imagine having multiple components open in your editor, and they're all namedindex.ts
. Your editor would likely add the directory as part of the filename to discern the two. For example, this is what it looks like in VSCode:
- Probably a bunch more reasons
The test might be a bit overkill, but all it really does is check for children and makes sure the snapshot is the same, so we're not doing a lot there.
You do raise a good point about having a utility package. I went ahead and created the components/common
directory for shared components. So far I've moved the Overlay
and Link
components there lol
height: { | ||
'napari-app-bar': '75px', | ||
}, | ||
|
||
gridTemplateColumns: { | ||
'napari-nav-mobile': 'min-content 1fr', |
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.
what's this do?
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 creates a 2 column grid where the first column is only as wide as the content, and the second column fills up the rest of the space.
Interestingly the result would have been the same if we used min-content
or max-content
because we're purposely not wrapping the header.
Fractional units are interesting, but they basically specify what fraction of the remaining space should a grid column take. For example:
- If the grid is one column with
1fr
, then the column will take100%
of the space - If the grid is two columns with
1fr 1fr
, then both columns will take50%
of the space - If the grid is two columns with
1fr 2fr
, then the first column will take33%
of the space and the second column will take67%
of the space
It's mostly a convenience over having to manually calculate the percentages for grid columns. I included some references for more info below:
References:
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.
oh, so if I understand correctly, this is similar to flex weight then?
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.
If you're referring to flex-grow, then yes it is 😸
62d0f0e
to
947c9d6
Compare
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.
Looks great overall! I just added a minor comment.
interface Props { | ||
children: ReactNode; | ||
} | ||
|
||
export function Layout({ children }: Props) { | ||
return <main>{children}</main>; | ||
return ( | ||
<> |
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.
What does this empty tag do?
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 is known as a fragment, and it's used to return multiple elements for a single component.
React has a limitation where you can only return one element from a component. This used to lead developers having to return a lot of wrapper divs:
function SomeComponent() {
return (
<div>
<Foo />
<Bar />
</div>
);
}
Later on, React released Fragments so you could wrap multiple components under a single tag:
import { Fragment } from 'react';
function SomeComponent() {
return (
<Fragment>
<Foo />
<Bar />
</Fragment>
);
}
The shorthand syntax is the empty brackets, and is useful when you don't want to import the entire Fragment component:
function SomeComponent() {
return (
<>
<Foo />
<Bar />
</>
);
}
As a result, this will return the HTML for <Foo />
and <Bar />
without including a wrapper <div />
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.
Oh, cool! Thanks for explaining 😄
<button data-testid="drawerClose" onClick={onMenuClose} type="button"> | ||
<Image | ||
src="/icons/close.svg" | ||
alt="Icon for napari search bar" |
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.
Should be something like "Menu close button" instead?
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.
Sounds good, perhaps I was being too technical since it's not actually the button element, but it makes more sense to add it 😆
@justinelarsen @kne42 I made a change to the PR that moves almost all of the Tailwind utilities into the component class name. I did a comparison on the the CSS output, and doing it this way generates a smaller CSS file than having everything as its own class in a CSS module. I went ahead and removed most of the CSS module files, but left a couple since there's no equivalent in Tailwind. I think going forward, we should try to use Tailwind class names as much as possible, and reserve custom CSS for things Tailwind can't do. For example, using the |
Fine with me if it makes the files smaller. Would love to hear what @kne42 thinks about it. |
Another big reason is that the |
@@ -1,3 +1,4 @@ | |||
import clsx from 'clsx'; |
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.
what does clsx do? I'm not exactly sure I understand it completely from the package description...
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.
clsx
is a smaller alternative to classnames. I think you would understand the functionality of clsx
if you read that README instead.
Its purpose is to apply class names to a single string. For example, something like clsx('flex', 'flex-auto', 'justify-center')
will output 'flex flex-auto justify-center'
. It's mostly useful for conditionally applying styles. For example, if I want to add a visible
class if the isVisible
state is true
:
<div className={clsx('foobar', isVisible && 'visible')} />
For this PR, I found it's useful for Tailwind classes because you can split up extremely long class name strings into multiple lines, which was the main reason why I wanted to move it to SCSS originally.
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 is probably a good thing to talk about in the frontend workshop too 😄
Description
This PR implements the App Bar for the napari hub. The styling is very similar to napari/napari.github.io#122, using the same 2-column and 3-column layout for styling the app bar.
Changes
package.json
scripts, and making the viewport responsiveDemos
Slide Out Menu
menu-drawer.mp4
Responsive App Bar
app-bar-responsive.mp4
Demo Code
./src/pages/index.tsx
./src/pages/about.tsx
./src/pages/index.module.scss