-
Notifications
You must be signed in to change notification settings - Fork 325
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
feat: add welcome page #565
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.
@fsdiogo posted some of my content ideas in #564 (comment)
I've added a commit with browser.runtime.onInstalled
hook that opens the landing page on first install or when loaded as a temporary extension. It should make development easier :)
Note 1: I had to register this hook synchronously before anything else. Registering from async function missed the event.
Note 2: It is possible to use the same hook to react for update event (eg. to display release notes of new release), but that is something for a separate PR :)
Cool, thanks @lidel 🙂 |
Current status: I have to put thumbnails and style them in the videos section, and link them to YouTube. I tried embedding the videos but got an error due to the extension blocking something, I don't remember what exactly, have to check that again. After that I'll start working on the left side. Thoughts? |
Onboarding linksIt is very easy to digest, does not overwhelm the user, I like it a lot! Let's reorder onboarding links/sections to follow the growth of user from noobie to contributor :)
VideosYoutube player was probably blocked by the default CSP, which is very strict in WebExtensions in Firefox: It is possible to set more liberal CSP, but if you just embedd a thumbnail that opens a video in a new tab it should also be fine: <a href="https://www.youtube.com/watch?feature=player_embedded&v=YOUTUBE_VIDEO_ID_HERE
" target="_blank"><img src="https://img.youtube.com/vi/YOUTUBE_VIDEO_ID_HERE/0.jpg"
alt="IMAGE ALT TEXT HERE" width="240" height="180" border="0" /></a> Left sideHere's an idea: What if when HTTP API on default port is down ( @olizilla created a very nice UI for this in WebUI, see screenshots in ipfs/ipfs-webui#715, we could basically copy the idea and add third option "start embedded js-ipfs". Thoughts? |
I like the idea but my fear is that for new users it'll be more confusing than a meaningful choice. They won't have all the background info. I think right now we want people to run a separate daemon, so perhaps we should try focusing the messaging on that... what is the clearest way we can explain it. We could offer the choice, but I don't think it should be presented with a 50/50 visual weighting of the concepts. How does metamask deal with it? They don't have an embedded option, so it'd be great to grab some screenshots of their onboarding flow and add them to an issue on https://github.com/ipfs/ipfs-gui so we can learn from it. |
More generally, this is looking great @fsdiogo |
I'd like to give the users the option to choose the node to connect, but I also agree that it can be confusing. I'm afraid that the instructions will take a lot of space, I'd like this page to be as simple as possible 🤔 |
That's a reasonable fear. This section of companion has 3 jobs:
Where the user has no api running we must still "catch" them, but the most important thing is getting them to a working set up. Given that, it feels like we could have a layout that gives a little space to creating the "catch" and the rest to "How to get this working well" Where the user has a working set up, catching and inspiring can take up all the space, and we can drop the the "get it working" section. Power users who want to change the api they connect to can do so in the app settings. When Desktop is updated, we'd message it like "go install desktop, then install companion" to increase the chances of the user being able to use companion right away, but we have to deal with both scenarios. |
also of note, where you don't have working setup, we'd still pass through the "everything is working" flow once they fix the issue... the flow for a new user with no daemon is something like: "the catch" -> "how to get it working" --> ...user goes off and installs a thing... -> "hurray you did it!" + "inspire them / what's next". |
I like the page, but I was wondering if there aren't any more up to date videos to put there. That 'IPFS Alpha' videos seems a bit old. Or show some of the other apps/functionalities like Desktop or WebUI. |
I think we could grab something from IPFS Talks Playlist. Eg. a short one hands-on (~10min) and a longer with the bigger picture. |
- also when loaded as a temporary extension to make it easier to dev
e1fed83
to
62deab5
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.
Quick feedback:
- Use fonts from
ipfs-css
We need to ship own fonts because some linux users have fugly default ones :)- Add
class="sans-serif"
to body element to set default font - (optional)
class="montserrat"
can be used for headers, if it looks better
- Add
Left pane:
-
The cube should reflect the API state, just like it does in browser action popup (if online: add heartbeat animation, if offline: display gray logo)
Code can be mostly copied from:
https://github.com/ipfs-shipyard/ipfs-companion/blob/40fd1a7942aa410c572593c861fa7dc203f52a4e/add-on/src/popup/browser-action/header.js#L16-L23 -
Padding/whitespace looks bit off, somethign is out of balance
- try aligning text to center
Right pane:
- Reorder sections to 1) New to IPFS? 2) Got Questions? 3) Want to help?
- Reorder project icons 1) Multiformats 2) IPLD 3) libp2p
- Add some padding around videos so they don't overlap when viewport is tight
@lidel new update based on your feedback, thanks!
IPFS API 👍IPFS API 👎 |
- capitalized onboarding headers - improved onboarding copy, added Dicover section, extended last one - removed blue circles from videos, felt bit too distracting
@fsdiogo looks awesome! 👍 🥇 I've added some additional links to onboarding steps in right pane and made tiny UI changes in dca7932 (mostly to make it look better with fonts on Linux and for people with custom GTK/dark themes), please fetch it and check if it looks good on Mac. The only remaining thing before we merge this is adding |
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.
Wow, nearly done 👍 👀
Before we merge this, please remove render
and copy
from i18n keys, they make keys longer without providing actual context to translators :)
- fix for UI jitter when API state is unknown - fix translated span renderer
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.
@fsdiogo I fixed race condition during companion init, now the landing page opens after companion inits, so there will be no false-negatives.
While I was at it I also added:
- a fix for UI jitter when API state is unknown
- a fix for translated span renderer
Check if it looks ok on your end. Is there anything you want to add before we merge this?
Fixed it, commit incoming. Other than that I think it's ready to merge 🚀 |
Tested it in |
minor, but the vertical alignment of the left side is based on the height of the content on the right side rather than the height of the screen. On a 13" laptop screen the message on the left look a little low down and on a smaller screen the text gets cropped off the bottom. Would be great to fix that so it was centred on screen height, but could be a separate PR. |
Yeah, I think I see what you mean: @fsdiogo will you be able to address this before merge? :) |
I'm working on that, but having some issues. When the left side doesn't fit in the viewport I'm unable to have scroll there because I'm using |
Here's the thing: my idea was to fix the left side and give it a Solution: as we don't have a lot of content on the left side, it's possible to keep it all on the viewport, for screens with smaller So the left side will always be fully visible with no scroll, and the right side content will have scroll (the page scroll). Is that ok with you guys or were you thinking of another solution? |
I think it would be ok with me as long responsive reflow on narrow screens (eg. mobile) still works. I mean this (right side moved below left side due to the narrow viewport): If we can't support that, then its better to merge current version. |
Yep i get you. I'd do what you can to make the content on the left fit the available space on small screens without needing a scrollbar. If the left still overflows vertically, then it should probably get it's own scrollbar like https://jsbin.com/lehemay/1/edit?html,css,output |
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 @fsdiogo, thank you!
I'm gonna merge this and push to Beta to get some initial feedback while translation begins.
Check #564 for reference.