Gatsby starter #335
Gatsby starter #335
Conversation
Codecov Report
@@ Coverage Diff @@
## master #335 +/- ##
==========================================
+ Coverage 25.82% 27.05% +1.22%
==========================================
Files 152 160 +8
Lines 3729 3800 +71
Branches 397 414 +17
==========================================
+ Hits 963 1028 +65
+ Misses 2489 2483 -6
- Partials 277 289 +12
|
|
|
||
| componentDidMount() { | ||
| fetch( | ||
| 'https://raw.githubusercontent.com/gatsbyjs/gatsby/master/docs/starters.yml' |
There was a problem hiding this comment.
to put this in Algolia, we will have to push to Algolia somewhere https://github.com/gatsbyjs/gatsby/blob/master/www/gatsby-node.js here I think, but I'm not too sure yet what the most efficient way will be.
There was a problem hiding this comment.
Thanks, for pointing to the Graphql query in the Gatsby starter. It would be great if we could use a similar query but I'm not sure how to query the same data.
I think for now we can just use the starter.yml and I think the selection is OK now. I'll update the screenshots above in a minute.
Also the search can be easily done by filtering through the yml data. So Algolia is not really needed here at the moment.
There was a problem hiding this comment.
Overall this looks pretty good.
I did have one high-level concern, which is that I had a bit of a different flow in mind. I imagined that we'd restructure it into the following sequence:
Step 1. Enter name, type, icon
Step 2. Project-specific settings. For Vanilla React it would skip this step. For gatsby, it'd show a list of the most popular starters, or let the user enter their own
The benefits to this flow are the following:
- There's no modal-in-modal for the project-specific settings. I'm always a bit bothered when one modal prompts another modal to open, and the Redux setup really only expects 1 modal to be active at a time, so it's probably not the best user-experience.
- The initial screen, for name/type/icon, doesn't get too crowded, and doesn't change depending on which option is selected (well except the button text would change from "Let's do this" to "next").
- More flexible for other project-specific settings, without having to worry about running out of space.
Sorry I didn't make that potential flow known! I think it'd be a better experience, and I also think it'd simplify the code quite a bit... but I know you've invested a lot of time in this current approach and I'd understand if you want to ship it as-is now. We can always revisit later.
Other than that, I had some questions inline
|
|
||
| static getDerivedStateFromProps(nextProps: Props, prevState: State) { | ||
| return { | ||
| gatsbyStarter: nextProps.projectStarter, |
There was a problem hiding this comment.
Hm, so I think in general this is a not-ideal pattern. I feel like either this component should own the state entirely, so it doesn't receive projectStarter from props, or it should always delegate to the parent's projectStarter.
There was a problem hiding this comment.
I think you're right but with-out this it would require me to modify modal state as I need to have the info about the entered starter before opening the modal.
So the user can dismiss the changes made in the modal. Anyway I'm not changing this as I'll check if I can remove the second modal - so this won't be needed anymore.
There was a problem hiding this comment.
It's acutally an anti-pattern to override local state unconditionally with every prop change.
There is a nice post you-probably-dont-need-derived-state about this in the React.js blog.
We don't need it here anymore as I'm rewriting this. But I wanted to note this as I've learned this and I wasn't aware of it.
| gatsbyStarter: selectedStarter, | ||
| }, | ||
| () => { | ||
| console.log('updated', this.state); |
There was a problem hiding this comment.
I feel like we don't need this callback?
Also, there's another console.log above it.
There was a problem hiding this comment.
Yes, that's right. Sorry, I've missed that debug stuff. I'll remove it.
| starters library{' '} | ||
| <ExternalLink href="https://www.gatsbyjs.org/starters/"> | ||
| here. | ||
| </ExternalLink> |
There was a problem hiding this comment.
I think it's slightly more conventional to make the link inline:
For a better overview you can also have a look at the{' '}
<ExternalLink href="https://www.gatsbyjs.org/starters/">
Gatsby starters library
</ExternalLink>.
There was a problem hiding this comment.
OK, modified.
What I've noticed with the external links in the summary pane is that the link is not fully clickable. I think that's also happening on master so this is not introduced here.
Also the link visiblity is pretty bad as it's not visible that it's clickable. What do you think would it be OK to add a thin light rounded box around these links?
Or maybe underlining would also work similar to the import project button.
For the hover I need to check what's causing it - looks like the width of the container is not wide enough. I haven't found the cause yet.
Here is how this looks:
There was a problem hiding this comment.
Hover issue fixed with PR #337. It was an issue with the backface pointer events.
We could improve styling later - thin outline not added.
| class MainPane extends PureComponent<Props> { | ||
| type State = { | ||
| gatsbyStarter: string, // Temporary value during selection in selection toast | ||
| }; |
There was a problem hiding this comment.
hm it feels like this state lives in so many places... Couldn't this invoke updateFieldValue instead of being set locally?
There was a problem hiding this comment.
Yes, that's right - it lives in many places. But I'm not sure if I can simplify this. As the user can open the starter selection component and select a starter but then decides to cancel that selection so the previously entered starter is still available.
If I would use updateFieldValue that flow wouldn't work.
| const renderedSteps: Array<?React$Node> = buildSteps.slice( | ||
| 0, | ||
| currentStepIndex | ||
| ); |
There was a problem hiding this comment.
I feel like this might be a bit over-engineered... I think I'd opt for a "dumber" approach:
const steps = [];
if (currentStepIndex > 0) {
steps.push(/* first step */);
}
if (currentStep > 1 && projectType === 'gatsby') {
steps.push(/* project-specific step for Gatsby */);
}
if (currentStep > 2) {
steps.push(/* third step */);
}
In addition to being a bit easier to follow, it doesn't create a bunch of elements that might not be used.
There was a problem hiding this comment.
I think you're right. That's a bit over-engineered. :-) Your code is probably easier to follow. I'll have a look at it and maybe I re-write this but I have to think about how I'll implement your proposed flow with-out the additional modal.
So if I'm changing the step order I have to modify this part.
| return ( | ||
| (currentStepIndex > 0 && !projectType) || | ||
| (currentStepIndex >= lastIndex && !projectIcon) | ||
| ); |
There was a problem hiding this comment.
I'd love for this to be broken into a couple steps, so that the steps can be given variable names to convey meaning.
const needsProjectType = !projectType && currentStepIndex > 0;
const needsProjectIcon = !projectIcon && currentStepIndex >= lastIndex;
return needsProjectType || needsProjectIcon;Also, I was initially confused because I assumed validateField would return true if the field was invalid... how about isSubmitDisabled as the name?
There was a problem hiding this comment.
Good idea. Renaming to isSubmitDisabled makes it easier to understand. Splitting is also OK as it improves readability.
I first though that it's clear what these checks are doing but for new devs it's better to have that additional naming.
| @@ -185,6 +185,29 @@ class SummaryPane extends PureComponent<Props> { | |||
| </Fragment> | |||
| ); | |||
| } | |||
| case 'projectStarter': { | |||
| // todo: why is a key needed on FadeIn? Was s3t. | |||
| // todo: should we rename projectStarter to be mores specific as this is Gatbsy only. | |||
There was a problem hiding this comment.
Typo, Gatsby.
Also I don't understand the first todo, what's s3t?
There was a problem hiding this comment.
Oh, OK, I'll fix that typo.
The key="s3t" is in line 178 of SummaryPane. I haven't added it to the Gatsby part. Is the key needed or do I miss why it's there?
| > | ||
| <HoverableOutlineButton | ||
| noPadding | ||
| onMouseDown={() => window.requestAnimationFrame(handleFocus)} |
There was a problem hiding this comment.
Not sure if it's needed. It's copied it from ProjectName#line202.
I haven't tested it with-out it. We could refactor ProjectName component later to use the TextInputWithButton component.
There is just a visiual issue with the line below the icon (double line) as menitoned in the PR. Do you have an idea why the line appears below the icon?
|
|
||
| // We're using "template" variables inside the project type configuration file (config/project-types.js) | ||
| // so with the following function we can replace the string $port with the real port number e.g. 3000 | ||
| // (see type VariableMap for used mapping strings) |
There was a problem hiding this comment.
Hm, what's the advantage to doing it this way? We used to just pass the variables along to the function instead of needing to do substitution. Did adding starters increase the complexity here?
There was a problem hiding this comment.
No, starters don't add complexity here. I just wanted to clean this so it looks like at the devServer.
I think with this it's easier to add new project types as there is no callback in the project type configuration file. The substitution was added in PR #292.
|
I've tried to modify the steps as you mentioned (I'll push it in a minute) but I think there isn't enough space to add the starter behaviour as the last step (see screenshot below). I'd like to keep the notice to the Gatbsy starters, a input for searching (we can use the input we're already having) and a list with the search results + the Codesandbox preview buttons (maybe with a smaller icon). Maybe we could reduce the spacing between the steps. I'll have a look at it later today.
|
|
@joshwcomeau Please have a look at my updated code. I've addressed your review comments. I've added everything to the same modal & changed the order of the steps. Just the height jumps between the project types but I think that's OK or should we do equal heights? So CRA/Nextjs pane has the same height as Gatsby. Please let me know if I missed anything from your comments. |
| const WrapperLabel = styled.label` | ||
| const WrapperLabel = styled.label.attrs({ | ||
| // default to 30px spacing between FormFields | ||
| spacing: props => props.spacing || 30, |
There was a problem hiding this comment.
I'll move this to FormField component static defaultProps as it's only one location to add the default of 30 instead of every styled-component.
|
@joshwcomeau could you please have a look if this is working as expected? I think just the panel height could be improved but maybe we could change this in a following PR as this is already pretty large. |
|
@melanieseltzer could you please have a look at this? I think this is ready. |
|
This is looking pretty great 👍 I only took a surface level look so far to play around with it, haven't dug into the code yet. Seems like the select starter functionality is working, and the preview in Codesandbox functionality. Since this is quite a large PR I'm going to have to dedicate some more time to take a deeper look, but for now I have some preliminary notes:
I wonder if we could leverage the <BackgroundScroll disabled={isVisible} />
...
const BackgroundScroll = createGlobalStyle`
html, body {
overflow: ${props => (props.disabled ? 'hidden' : 'auto')};
}
`
So once the starter gets selected it gets pulled up to the top of the list, and out of the flow. This is somewhat confusing to me and I feel like it should stay in place and just be highlighted to indicate it's selected. Otherwise the user loses their place and has to scroll back down to get back to where they were, in case they want to make another selection. Actually I think there's an ordering bug when you make a selection 🤔 Unless I'm misinterpreting this flow, but it doesn't feel intentional. You can see what I mean in this GIF:
As you can see in the screen cap, I scroll to the bottom of the current list of items, and you can see the last two items are It seems like maybe the pagination is off? |
|
@melanieseltzer Thanks for your review.
|
👍
Ah, gotcha. That's the thing I wasn't understanding about the functionality. As someone just jumping into the flow, I didn't associate selecting a starter (clicking on its name) as setting a best match... I just thought I was making a selection choice, and not expecting the results to change based on that. That's why it's appearing jumbled to me. Is there a reason to have a 'best match' functionality? As a user I'm expecting search results to be pretty static:
It is somewhat problematic for the input text to change to the URL when a selection is made. Somewhat annoying to have typed 'blog' and then after the selection is made, the text is wiped. Cause if you make a selection prematurely but change your mind, you'll have to type 'blog' in again and it's an extra step. tl;dr Keeping in mind I'm not a UX expert or anything, just going by how I interact with things. So @joshwcomeau and others please feel free to jump in here 🙂 I think I would suggest:
|
|
@melanieseltzer you're right, the re-ordering things is a bit weird. I like your proposed UX flow just one thing I'd like to have - moving the selected starter to the top of the list. So I've changed it as you proposed except modifiy list (not pushed yet):
Indication of selected starter Loading of starters Removing selected starter I've added it by clicking on the selected starter a second time - this will deselect the starter. Is this OK or do we need a separate button to deselect the starter? |
|
@melanieseltzer Please have a look if this is OK and I have addressed everything. I think we can revisit and optimize this later. I think the reordering of selected starter to the top is not perfect but OK for now. It would be better to keep the order and have a separate display of the selected starter. (Needs to be in a separate location because the typeahead could change the list and won't display the selected starter.) Also list size is a bit small with just two items but I don't have an idea at the moment how we could easily optimize this. An idea would be to have icons for each build step and only display the active step or we could make the project icon selector collapsilble (we would probably get just one more item in the starter list with this as we could collapse to one icon). Scrolling of background with active modal disabled with |
| <StarterItemHeading | ||
| selected={selectedStarter === starter.repo} | ||
| onClick={() => | ||
| this.handleUpdateStarter( |
There was a problem hiding this comment.
There's a lot of passing down/abstraction of the updateFieldValue function (from CreateNewProjectWizard) to ultimately end up here.
The chain right now seems to be:
CreateNewProjectWizardpasses it toMainPaneasupdateFieldValuepropMainPanepasses it toProjectStarterasonSelectpropProjectStarterpasses it toSelectStarterListasupdateStarterpropSelectStarterListabstracts it into functionhandleUpdateStarter
This was a bit hard to follow when I was in ProjectStarter and wanted to see where handleOnSelect was getting its parameters set.
I wonder if there's a way to simplify this or at least keep the naming consistent when passing it around?
There was a problem hiding this comment.
Yes, you're right. This is pretty complex. A quick-fix after merging this would be to name it consistently.
But we could also open an issue to check if we can simplify this with Context-API or a Reducer - so we don't have to pass it through each component.
I think the ProjectStarter uses one more child component compared to the other build steps.
There was a problem hiding this comment.
This flows much better 👍 I've just a concern about passing functions down but it's something we could optimize later.
move selected starter to top of list - I think that's needed as there are cases you won't see that you have selected a starter
Good point, I hadn't thought of that. I think it works well now that there's no re-ordering of the results. I'm no longer confused.
We won't hide the starters if typeahead is empty
That's a good idea, I like it.
I've added it by clicking on the selected starter a second time - this will deselect the starter. Is this OK or do we need a separate button to deselect the starter?
Yeah I think this is okay!
The disabling of background scroll when modal is looking good 👍
|
@melanieseltzer Or should we add that feature back in? E.g. if the user enteres a matching starter by name or url it will automatically pick the starter? |
|
@AWolf81 Oh good point! I'll wait for the new PR because I've also identified an issue with search results that I'd like to iterate over. |
Related Issue:
#47
Summary:
Add a Gatsby Starter selection to
CreateNewProjectWizard& add the business logic to pass the starter url to the create command.I've enlarged the TwoPanelModal height so the new component will fit into it.
The thing with selecting more stuff is something we're handling with Guppy plugins. So we can add Flow, Storybook, Docz,... after project creation to keep the setup small.
Screenshots/GIFs:
Gatsby with starter
Starter selection list(Removed and added as requested)Todos:
DirectroyPickerProjectStarterSelectorcomponent and use it inprojectSpecificStepsmethodSelectStarterDialog- Not working at the moment.config-variables.servicestarter.ymlnot sure where to get the image - I'll check the Gatsby starter selection page. Right now not possible as the yml doesn't contain a screenshot.MainPane. Button disabling & Form submitting not working as expected.Discussion
Deploy to Netlifybutton? Or is this confusing for beginners? I think it's easy to add but I'm not sure if we should add it. (Just opening a link like https://app.netlify.com/start/deploy?repository=https://github.com/gatsbyjs/gatsby-starter-blog will deploy to netlify).https://github.com/gatsbyjs/if no url is entered - so enteringgatsby-starter-blogwill use the starterhttps://github.com/gatsbyjs/gatsby-starter-blog. We could also add a replacement for username/repo so it would behttps://github.com/username/repo.Note
There is a visual issue (double border at the bottom) at the HoverableOutlineButton in combination with TextInput. It's also on master as you can see in the screenshot below.
So it's not introduced here. I've tried to fix it but I couldn't find the cause of it. If I remove the
border-bottomfrom wrapper inTextInputthere is no border anymore so I think the icon will somehow inherit the border from that wrapper.The border is more visible on narrow screen width.