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!: new website frontend and app ui #1001
Conversation
Chore: Duplicate Next app
…app code feat: updated login and logout urls feat: added path /app/logout to allow logout from /website feat: added path /app/api/auth to retrieve logged in status for /website
feat: Storybook installation feat: Updated README with storybook information feat: added css module support
feat: Base styles
feat: Updated storybook compiler feat: Updated button component functionality
…-initial-refactor
Porting over initial refactor updates
Most of my comments so far are nitpicks that don't need to block this PR from landing if we are keen to get it live asap. The only blocker currently is the |
Cool, removed that directory! It was just there for easy reference without jumping between commit hashes. The only thing that I do see missing comparing the current site and this site is the link to Filfox URLs for each Filecoin SP. |
|
||
export default function Custom404() { | ||
useEffect(() => { | ||
countly.trackEvent(countly.events.NOT_FOUND, { |
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.
@orvn do we track 404s in countly somewhere else? Looks like it's been removed from this 404 page.
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.
No, this wasn't intentional—thanks for catching. We had some issues with the 404 at one point and this particular Countly event needs to be added back (we did a sweep of Countly throughout the site to ensure everything was still in there, but this particular one must have been missed).
NotesIt looks lovely and the attention to detail is great. I like the pattern of splitting out the content as .json files, makes it easier to edit copy and see the words for the Visually, it's a delight. I have a concern that logged in view of the app doesn't give enough space and focus to the users uploaded file listing, which i see as the main thing for this service. The file list feels tight / constrained and a little uncomfortable to use right now. We can iterate on this. I have a non-blocking issue, and I feel ridiculous raising this, but I'm a little sad that the linting rules for the website are diverging further from those of the rest of the project. It doesn't feel like the JS in this project is trying to fit in with the prevailing style of the rest. This is a Questions
TODOat a minimum we still need to fix these snags or explain why they are needed
you will win more of my gratitude and affection for responding to or fixing other snags commented inline before merging. |
Thanks for the review! Some of the fixes or enhancements are in PRs. Will merge them into this branch. Currently a cleanup of the readme files and legacy content in this PR as well. |
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.
Unfortunately I can't npm install
on my M1 Mac:
npm ERR! code 1
npm ERR! path /Users/alan/Code/web3-storage/web3.storage/node_modules/optipng-bin
npm ERR! command failed
npm ERR! command sh -c node lib/install.js
npm ERR! node:internal/child_process:413
npm ERR! throw errnoException(err, 'spawn');
npm ERR! ^
npm ERR!
npm ERR! Error: spawn Unknown system error -86
npm ERR! at ChildProcess.spawn (node:internal/child_process:413:11)
npm ERR! at spawn (node:child_process:700:9)
npm ERR! at /Users/alan/Code/web3-storage/web3.storage/node_modules/bin-check/index.js:22:12
npm ERR! at /Users/alan/Code/web3-storage/web3.storage/node_modules/executable/index.js:27:4
npm ERR! at FSReqCallback.oncomplete (node:fs:199:5) {
npm ERR! errno: -86,
npm ERR! code: 'Unknown system error -86',
npm ERR! syscall: 'spawn'
npm ERR! }
I'll look into this and maybe come back with more feedback once I get the site running locally.
In the mean time, there's a few things @olizilla has pointed out that need to be addressed before merging:
- Website: remove website/.github/pull_request_template.md
- Website: remove .legacy directory
- Website: revert the version number change in package.json
- Website: revert change to packages/client/package-lock.json
- Website: revert change to packages/client/tsconfig.json
Please create an issue (or multiple if you prefer) for the following snags and we’ll tackle them after the main PR lands:
- Docs: Algolia - make it run after deploy instead of on a cron
- Docs: Github workflows move to root and ensure are working
- Docs: Remove submodules
- Website: remove all references to yarn - this is an npm project
- Website/Docs: lint with standard for consistency with the other packages
- Website: some typesccript components - please convert to JS so we have a consistent language choice
- Website: Remove verbose commenting. It makes sense in the SCSS to group styles that apply to the same subject but everywhere else it seems to be describing very obvious sections of the code and does not add any value and we don’t do this anywhere else. e.g.
// ===================================================================== Imports
// ///////////////////////////////////////////////////////////////////// General
// -----------------------------------------------------------------------------
- Website: fix regression: 404’s are no longer tracked in countly
- Website: remove images with “-legacy” suffix
packages/website/package.json
Outdated
@@ -1,103 +1,94 @@ | |||
{ | |||
"name": "@web3-storage/website", | |||
"version": "1.10.0", | |||
"version": "2.0.0", |
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.
Please revert - release please will increment this, just ensure when merging the commit message contains a "!" i.e. feat!: new website frontend and app ui
.
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.
Ah, got it. Reverted and changed this PR title to feat!:
format. So I assume this will be the squashed commit title and get incremented by release please as a major version?
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.
yes exactly
"clsx": "^1.1.1", | ||
"countly-sdk-web": "^20.11.2", | ||
"filesize": "^6.1.0", | ||
"gray-matter": "^4.0.3", | ||
"highlight.js": "11.4.0", |
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.
Is this pinned to this version specifically for a reason?
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.
At one point it was causing some build issues, but only for users running locally on Windows machines. Will try to unpin and have someone test again
Fix for M1 issue: #1120 Additional feedback: Blocking:
Later:
|
* feat: add checkmark overlay on dropdown select * feat: add checkmark overlay on dropdown select * fix: modal animations * feat: animating ellipsis * feat: pass in alt to img block * fix: checkmark animation * fix: checkbox always checked * feat: Limit number of storage providers to five with a toggle * feat: Limit number of storage providers to five with a toggle * fix: Fixing pagination and order scrolling (#1108) * docs: add example of > 5 SPs to mock api * fix: Fixing type error * feat: add ie11 banner flag * fix: ie11 and had checkmark on refresh * fix: ie11 and had checkmark on refresh * fix: browser update script * fix: ts error * fix: use mixin for ie11 and add min height to table * fix: missing link effect on SP addresses Co-authored-by: MichaelPhan <michael.d.phan@gmail.com> Co-authored-by: orun <orun@agencyundone.com>
* feat: New UX token create improvements * chore: Better variable naming * cleanup: remove legacy codebase for site and docs * style: change confirmation symbol for token creation * style: make token input more prominent when focused Co-authored-by: Michael Phan <michael.d.phan@gmail.com>
Update
Additionally, the following recent enhancements were merged in (these were not critical for launch, just what we've been working on recently):
|
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.
Ok so, the big issue I have is that the docs do not retain the logged in state. I realised this yesterday and listed it as a blocker.
It's the primary reason we moved from the subdomain so I'm disappointed it hasn't been considered.
I think we're going to have to launch with this as a known issue. It needs to be addressed with high priority ASAP though.
Many thanks for all the work, it looks incredible 🚀
Thanks @alanshaw! Regarding the docs, there wasn't any particularly stable way to integrate them into the Next site in their current form (Docusaurus and the HTTP API in ReDoc). After a chat with @yusefnapora, we proceeded with the reskin of all the styles, but didn't really spend time on all the visual details within it. The next (no pun intended) step is to use something like Nextra to bring the docs into the Next static app (will open a new issue for this). Edit: issue added here #1138 |
Ships new NextJS website including:
See commit list for details, since this is an initiative with a large collection of epics