chore(ApplicationLauncher): Convert examples to typescript #6834
Conversation
| @@ -0,0 +1,4 @@ | |||
| declare module '*.svg' { | |||
There was a problem hiding this comment.
Reason for this is mentioned here: #6791 (comment)
jpuzz0
force-pushed
the
chore/application-launcher-examples-to-ts
branch
from
8f0cf45
to
b8a4605
Compare
nicolethoen
requested review from
nicolethoen,
kmcfaul and
wise-king-sullyman
packages/react-core/src/components/ApplicationLauncher/examples/ApplicationLauncherBasic.tsx
Outdated
Show resolved
Hide resolved
| const onToggle = React.useCallback((isOpen: boolean) => setIsOpen(isOpen), []); | ||
| const onSelect = React.useCallback((_event: any) => setIsOpen(prevIsOpen => !prevIsOpen), []); |
There was a problem hiding this comment.
I know that we discussed the optimization of examples as we work through these conversions, and the conclusion I remember being reached was that it was up to each of us to do as we see fit.
I debate though whether the usage of useCallback in situations like this is actually optimization or just excessive pre-optimization that doesn't actually improve performance, and may in fact decrease performance. I tend to agree with the arguments made by Kent C. Dodds in this article of his.
That being said my mind isn't hard set on that.
There was a problem hiding this comment.
That's a great article for sure and a good discussion to have. I think in this case, you're right in that they're not needed and even though there's a possibility of these functions being used for side-effects that would require referential equality checks, probably best to not make that assumption so I'll update.
| ); | ||
| } | ||
| } | ||
| ```ts file="./ApplicationLauncherMenu.tsx" |
There was a problem hiding this comment.
I see you've got some notable differences here between example names and component/file names. I can't remember if the team came to an alignment on this topic at the last meeting, unfortunately, but if not I feel like we should. I know that @thatblindgeye and I proposed a standard of component name + example name that matches 1:1, and I recall some debate on whether the name of the component was necessary but that's all I can recall at the moment.
I do understand (especially with the context of this example name) why you made the decisions you did though, and if we want to just standardize on something looser like component name + something that uniquely identifies the example I don't think I'd be opposed to that.
There was a problem hiding this comment.
Oh I see, I didn't realize we had said exactly the name of the example, versus something that resembles it (this leads to fewer characters in most cases), which is what I was aiming for. I'm fine with re-opening the discussion.
There was a problem hiding this comment.
I know that was our proposal, but I can't recall if there was a consensus reached on it and regretfully I didn't take notes. I do think some examples (like the one for the file I pointed out) have names that just don't work well for this purpose.
@thatblindgeye do you have any thoughts?
There was a problem hiding this comment.
I think componentName[exampleName] can work most of the time, but falling back to componentName[shortenedExampleName] would be good to have especially when it comes to something like "Basic with menu appended to document body". I think it's really just going to have to be a "use your best judgment" thing on which one to use and which best describes the example itself (in this instance, for example, I would say ApplicationLauncherDocumentBody or similar might be a more fitting name).
We could tweak the example name to something like "Menu appended to document body", but that would still result in a rather lengthy file/example name of ApplicationLauncherMenuAppendedToDocumentBody (give or take a few characters if this is shortened slightly further). This is definitely an example we knew would come up to show just how lengthy these names can get.
If we'd want the verbiage written down anywhere to actually reference I can write something a little more formal up as well, or help work on that.
There was a problem hiding this comment.
I agree that shortening the example part of the name would be good to keep the overall length of the filename down. What do you think of abbreviating the component name where applicable as well (AppLaunch or AppLauncher in this case), or dropping the component name section altogether because it isn't directly user displayed and can be inferred from the file structure?
There was a problem hiding this comment.
I had suggested the same as I thought the name of the component was redundant, but I don't recall there being much of a reception to that when discussed in standup a week ago.
| </ApplicationLauncherItem> | ||
| ]; | ||
|
|
||
| export const ApplicationLauncherMenu: React.FunctionComponent = () => { |
There was a problem hiding this comment.
Based on the conversation regarding naming, I'm wondering if this name should instead revert back to ApplicationLauncherDocumentBody instead of ApplicationLauncherMenu, or if that does stray into "too long" territory.
There was a problem hiding this comment.
At the risk of being excessively pedantic: aligned right, aligned top, and some of the with examples don't match the name of the examples 1:1 despite their lengths not being excessive if they did IMO.
These are just small nits though and "too long" is, of course, subjective so none of these are blockers for me. I just didn't feel right not pointing it out when I noticed it, since it doesn't quite match up with my understanding of the consensus we reached on this topic earlier today.
What: Closes #6797