Skip to content
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: Moving form and result to two cols #47

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

Conversation

ImTheCodeFarmer
Copy link
Contributor

@ImTheCodeFarmer ImTheCodeFarmer commented Aug 15, 2022

Still a WIP but moving the form and results to two columns. Need to clean the code a bit and take care of the case where the result is really short. This causes a large gap between the form and result.

Chad's checklist of things to get looking 💯

@github-actions
Copy link

github-actions bot commented Aug 15, 2022

PR Preview Action v1.1.1
🚀 Deployed preview to https://raendev.github.io/admin/pr-preview/pr-47/
on branch gh-pages at 2022-09-30 15:04 UTC

@willemneal
Copy link
Member

@jhammond2012 @chadoh It seems that the preview isn't working for me in brave or firefox.

@willemneal willemneal requested a review from chadoh August 15, 2022 15:19
Copy link
Contributor

@chadoh chadoh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can't figure out why the PR Preview isn't showing up, but I was able to look at it locally. It's looking pretty good. My biggest current concerns are:

  • most pages are now too wide, since the --max-width variable is used by container, which is used all over, such as on the home page
  • feels pretty jumpy. for example, flashes of wide content before content finishes loading, followed by a repaint to narrow layout. even in the best cases, this feels a little disorienting to me. This might be hard to fix in the general case; I'm up to have a call to chat through possible solutions.

src/styles/global.scss Outdated Show resolved Hide resolved
src/index.tsx Outdated Show resolved Hide resolved
src/components/Form/form.scss Outdated Show resolved Hide resolved
@ImTheCodeFarmer
Copy link
Contributor Author

@chadoh I moved the container class around a little bit so I could dynamically add wide to the class. Have a look at let me know if you feel this solution is okay.

@chadoh
Copy link
Contributor

chadoh commented Aug 22, 2022

This one still jumps at first page load: http://localhost:3000/#/v2.tenk.testnet/RemainingAllowance

@chadoh
Copy link
Contributor

chadoh commented Aug 22, 2022

I added a list of things to check to the PR description, and checked off the ones that look good to me.

The jumping is much more tolerable in this version, since it happens less, and only at more expectable times. Still noticing it here and there though, as mentioned above.

You'll notice various issues as you click through. Some of the biggest ones currently worth noting:

  • loading wheel no longer shown during network request
  • error messages not shown at all

@chadoh
Copy link
Contributor

chadoh commented Aug 22, 2022

why so many yarn.lock changes? no corresponding package.json changes are present

@ImTheCodeFarmer
Copy link
Contributor Author

why so many yarn.lock changes? no corresponding package.json changes are present

That's a good question. Did you use NPM before instead? I haven't installed/changed anything regarding that. Only ran yarn install

@chadoh
Copy link
Contributor

chadoh commented Sep 30, 2022

@jhammond2012 ah, right, this repo is not using yarn. I tried to find the old commit where we switched to npm. At the time it fixed a deploy bug; I think it might have been related to PR Preview deploys when using Gatsby or something like that.

We're not using that stack anymore and could probably switch back to Yarn, but let's do that in its own PR and remove the yarn.lock file for now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants