Skip to content

feat: Moving form and result to two cols#47

Open
ImTheCodeFarmer wants to merge 5 commits into
mainfrom
twocol
Open

feat: Moving form and result to two cols#47
ImTheCodeFarmer wants to merge 5 commits into
mainfrom
twocol

Conversation

@ImTheCodeFarmer

@ImTheCodeFarmer ImTheCodeFarmer commented Aug 15, 2022

Copy link
Copy Markdown

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

github-actions Bot commented Aug 15, 2022

Copy link
Copy Markdown
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
Copy Markdown
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

@chadoh chadoh left a comment

Copy link
Copy Markdown
Contributor

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.

Comment thread src/styles/global.scss Outdated
Comment thread src/index.tsx Outdated
Comment thread src/components/Form/form.scss Outdated
@ImTheCodeFarmer

Copy link
Copy Markdown
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

chadoh commented Aug 22, 2022

Copy link
Copy Markdown
Contributor

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

@chadoh

chadoh commented Aug 22, 2022

Copy link
Copy Markdown
Contributor

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

chadoh commented Aug 22, 2022

Copy link
Copy Markdown
Contributor

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

@ImTheCodeFarmer

Copy link
Copy Markdown
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

chadoh commented Sep 30, 2022

Copy link
Copy Markdown
Contributor

@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