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

Replace headings with list items in balance warnings #4228

Merged
merged 5 commits into from
Apr 3, 2025

Conversation

ahmed-mgd
Copy link
Contributor

Fixes #990

Copy link
Contributor

@johrstrom johrstrom left a 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.

image

@johrstrom
Copy link
Contributor

This is still 2 columns. Feel free to radically change the structure of this html and perhaps the partials - indeed that is what we want.

If I edit the html on the page - this is close to what I'm looking for:

image

@ahmed-mgd
Copy link
Contributor Author

This is still 2 columns

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!

@johrstrom
Copy link
Contributor

This is still 2 columns

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.

@johrstrom
Copy link
Contributor

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.

@johrstrom johrstrom self-requested a review April 3, 2025 14:30
Copy link
Contributor

@johrstrom johrstrom left a 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!

@johrstrom johrstrom merged commit b154b93 into master Apr 3, 2025
39 of 40 checks passed
@johrstrom johrstrom deleted the balance-warnings-update branch April 3, 2025 14:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

update balance warnings visually and for accessability
3 participants