-
Notifications
You must be signed in to change notification settings - Fork 126
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
Replace headings with list items in balance warnings #4228
Conversation
apps/dashboard/app/views/shared/_insufficient_balance_resource.html.erb
Outdated
Show resolved
Hide resolved
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.
This looks better - but it's still 2 separate columns of text. I think it needs to read left to right top to bottom as if it were 1 single page in a book. Currently it reads like reading 2 separate book pages: left to middle, top to bottom (the first column), then middle to right, top to bottom (the second column).
If that makes any sense - it's hard to talk about text flow here, but clearly there are 2 columns of text with an invisible break in the middle, where it actually should read like 1 single row/column.
apps/dashboard/app/views/shared/_insufficient_balance_resource.html.erb
Outdated
Show resolved
Hide resolved
I believe I had a similar result to your screenshot, but I can double check that now. Just looking at the semantics though, it's unclear to me how this would produce two columns since it's a series of (mostly) single-level block statements. Sorry for the late reply and the misunderstanding over such a trivial issue! |
No issues on delays. Honestly it may have been that I didn't pull down the most recent updates? That's happened before, I keep commenting on the same commit even if you've updated it 🤦♂️ . I'll pull it down again and confirm. |
Yea I don't think I actually pulled the 4775035 because just now when I pulled I got 2 commits. So that's my bad, I was still reviewing the old commits. OK - I think we should just center the text and we're good to go. Right now it's left justified, I think center justification may look nicer. |
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.
Thanks for working through all the back and forth - and for forgiving my mistake in not pulling your changes!
Fixes #990