teacher tool - set module css pattern #9832
Conversation
|
I wanted to get these changes out so I can switch gears back to wrangling target theme colors into css variables. |
| children: React.ReactNode; | ||
| } | ||
|
|
||
| const VerticalSplit: React.FC<IProps> = ({ className, split, children }) => { |
There was a problem hiding this comment.
I want to switch away from the default export pattern for react components. Makes it too easy for actual component names to go stale, like this one. Something for a future PR.
There was a problem hiding this comment.
I'm a little confused about this comment because all of the components in this PR are using the export default .. pattern at the bottom of the file. Were we using the export default in the component declaration and you're wanting to use the export default pattern separate from the component declaration?
There was a problem hiding this comment.
I think we shouldn't export default components at all, so that we have to import them by their given name. It's the difference between this:
import SplitPane from "./SplitPane";and this:
import { SplitPane } from "./SplitPane";I'm suggesting we go with the second option, because it will enforce component name consistency. With the first option, this is equally valid:
import Button from "./SplitPane";The result is you've imported SplitPane and called it Button. We should close off this vector for confusion.
| @@ -44,6 +44,10 @@ | |||
| --high-contrast-hyperlink: #807FFF; | |||
| } | |||
|
|
|||
| * { | |||
There was a problem hiding this comment.
This is to reset some global styling coming from the target css that was messing up default div layout.
| children: React.ReactNode; | ||
| } | ||
|
|
||
| const SplitPane: React.FC<IProps> = ({ className, split, children }) => { |
There was a problem hiding this comment.
react-common has a splitter already. i will combine them eventually.
| @@ -46,7 +46,7 @@ export function fireClickOnEnter(e: React.KeyboardEvent<HTMLElement>) { | |||
| } | |||
| } | |||
|
|
|||
| export function classList(...classes: string[]) { | |||
| export function classList(...classes: (string | undefined)[]) { | |||
There was a problem hiding this comment.
What does allowing an entry to be undefined allow us to do?
There was a problem hiding this comment.
This was always the intended functionality of this method, but teachertool uses strict typing and so I had to make it explicit.
There was a problem hiding this comment.
The method filters out undefined elements.
| @@ -54,17 +51,12 @@ function App() { | |||
| <div className="ui large main loader msft"></div> | |||
| </div> | |||
| ) : ( | |||
| <div className="app-container"> | |||
| <> | |||
There was a problem hiding this comment.
What's the benefit of making this an empty tag rather than a div?
There was a problem hiding this comment.
The empty tag is shorthand for React Fragment. I moved the functionality this div was providing into MainPanel. But we still need a container here as a workaround for React's limitation of being unable to render a list of nodes at the top level of a component. React Fragments well-suited for this, because they make clear that the purpose of the node isn't to express UI, but to act solely as a container.
There was a problem hiding this comment.
Oh, interesting, thanks for the information!
| <div className={css["main-panel"]}> | ||
| <SplitPane split={"vertical"} defaultSize={"80%"} primary={"left"}> | ||
| {/* Left side */} | ||
| <> |
There was a problem hiding this comment.
Similar question to as above. Why do we want empty tags in components? Would having something like this cause problems for screen readers?
There was a problem hiding this comment.
This is another instance of needing a logical container that effectively disappears at runtime. In this case, each fragment acts as an envelope to deliver content to each side of the split pane. After that, their job is done.
Slight concern it will make debugging css a bit more difficult since the actual class names are different from the ones in the code, but willing to try it out and see if that's actually the case. |
The generated class names are usually readable enough to figure out where they came from. css sourcemaps would be great. I don't think they exist. Here's what |
|
Going to go ahead and merge this. At always, please feel free to leave feedback on the closed PR and I will address it! |
The main reason for this PR is to introduce "module css". This is a feature we get with create-react-app that scopes css class names to their module, making it easier to use simple names for css classes and they won't clash with one another.
Module css docs:
This PR contains some React changes too, but it is work in progress and will likely change a lot.