joshwcomeau / guppy

🐠A friendly application manager and task runner for React.js
ISC License
3.27k stars 154 forks source link

Gatsby starter #335

Closed AWolf81 closed 5 years ago

AWolf81 commented 5 years ago

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 grafik

Starter selection list (Removed and added as requested) grafik

Todos:

Discussion

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-bottom from wrapper in TextInput there 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. grafik

codecov[bot] commented 5 years ago

Codecov Report

Merging #335 into master will increase coverage by 1.22%. The diff coverage is 48.78%.

Impacted file tree graph

@@            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
Impacted Files Coverage Δ
src/config/project-types.js 100% <ø> (+25%) :arrow_up:
src/reducers/modal.reducer.js 100% <ø> (ø) :arrow_up:
...c/components/CreateNewProjectWizard/ProjectName.js 0% <ø> (ø) :arrow_up:
src/components/CreateNewProjectWizard/BuildPane.js 0% <0%> (ø) :arrow_up:
...s/CreateNewProjectWizard/CreateNewProjectWizard.js 0% <0%> (ø) :arrow_up:
src/services/check-if-url-exists.service.js 0% <0%> (ø)
...ependencySearchResult/AddDependencySearchResult.js 0% <0%> (ø) :arrow_up:
src/components/CreateNewProjectWizard/MainPane.js 0% <0%> (ø) :arrow_up:
...c/components/CreateNewProjectWizard/SummaryPane.js 0% <0%> (ø) :arrow_up:
src/components/FormField/FormField.js 0% <0%> (ø) :arrow_up:
... and 23 more
AWolf81 commented 5 years ago

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.

grafik

AWolf81 commented 5 years ago

@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. Spacing between the steps is reduced but I think it's still OK. Starter section looks OK (see updated screenshot). It would be better to have more than two starters visible in the list but I think there isn't more space available. Fontsize of the starter selection list is pretty small but similar to the other fonts used. Typeahead and highlighting is also improved and I like how it's working now.

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.

AWolf81 commented 5 years ago

@joshwcomeau could you please have a look if this is working as expected? If anything is open, please let me know.

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.

AWolf81 commented 5 years ago

@melanieseltzer could you please have a look at this? I think this is ready. I've change the height of the modal so it's the same for all project types now. I think that's better.

melanieseltzer commented 5 years ago

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 isVisible prop in CreateNewProjectWizard to disable background scrolling when the modal is open? Maybe with a conditional createGlobalStyle once styled-components v4 lands in #269? I'm imagining something like the following, but let me know if you have another idea:

<BackgroundScroll disabled={isVisible}  />

...

const BackgroundScroll = createGlobalStyle`
  html, body {
    overflow: ${props => (props.disabled ? 'hidden' : 'auto')};
  }
`
webpackHotDevClient.js:138 ./node_modules/encoding/lib/iconv-loader.js
9:10-32 Critical dependency: the request of a dependency is an expression
screen shot 2019-01-17 at 10 35 56 pm

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 gatsby-starter-mate and gatsby-minimal-portfolio-blog. So when I choose gatsby-minimal-portfolio-blog it gets pulled to the top, but when I scroll back down I'm expecting gatsby-starter-mate to be the last in the list. But it's not, it's nowhere to be found in that positioning.

It seems like maybe the pagination is off?

AWolf81 commented 5 years ago

@melanieseltzer Thanks for your review.

melanieseltzer commented 5 years ago

Maybe we could disable pointer-events on app wrapper if the modal is active to avoid scrolling.

👍

UX issue: Once you selected a starter that's the best match and the other result are having a similar url like the picked starter.

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:

I like to show the picked starter url in the input field

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:

AWolf81 commented 5 years ago

@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 If the user selects a starter and then clears the typeahead input it's not clear that there is a starter selected. OK, I have another solution to this. We won't hide the starters if typeahead is empty & toggling starter list to hidden is disabled if there is a selected starter.

Loading of starters How should we handle a missing internet connection? I first thought that it would be good to have something like in the screenshot below. But a missing internet connection is a general problem for project creation and we should display that earlier (see console log below unhandled promise after clicking build with-out internet). I think disable build button & show a message like Can't create a new project with-out internet connection and/or a popover / heading on the modal with that info. I think I'll remove the info from my working directory and we have to add this in a separate issue (issue not created yet). I think it's OK to fail silently for now. I'll create the issue in a minute. grafik grafik

Removing selected starter Because of the change of not adding the selected starter to the input box we need to have a way to remove the selected starter. Before it was handled by clearing the input field - which was also not perfect.

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?

AWolf81 commented 5 years ago

@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 overflow: hidden as mentioned here. That's working just the disapearing of the scrolls with active modal is not perfect but I think that's acceptable.

AWolf81 commented 5 years ago

@melanieseltzer Oh, one point I've missed is that we have to update the SummaryPane for the starter as the user have to pick a starter from the list as entering the starter url into the input won't select the starter.

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? If the user enters a url that's not in the starters list it will be picked & used on build click - e.g. user wants to use an unlisted starter. I'll add this and create a new PR for it.

melanieseltzer commented 5 years ago

@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.