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

Touch up SGB packet documentation #537

Merged
merged 1 commit into from
Feb 1, 2024
Merged

Touch up SGB packet documentation #537

merged 1 commit into from
Feb 1, 2024

Conversation

ISSOtm
Copy link
Member

@ISSOtm ISSOtm commented Jan 15, 2024

Turn the diagram into a SVG, and reformat/improve the doc.

Turn the diagram into a SVG, and reformat/improve the doc
@ISSOtm ISSOtm added content Improvements or additions to documentation consistency - style Content format, text style, consistency in presenting the informations labels Jan 15, 2024
@ISSOtm ISSOtm requested a review from avivace January 15, 2024 10:05
Copy link
Member

@avivace avivace left a comment

Choose a reason for hiding this comment

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

It seems like the base text size is a bit too big compared to the rest, it should be 120-140% of the base.

image

@ISSOtm
Copy link
Member Author

ISSOtm commented Jan 17, 2024

Being that this is a SVG which automatically scales to the device width, there isn't really control over that. Compare Firefox's "Responsive" view simulating a phone screen:

Screenshot showing the diagram with a wildly different proportion

Is it a big deal?

@avivace
Copy link
Member

avivace commented Jan 17, 2024

I think shouldn't, check for example how https://gbdev.io/pandocs/Rendering.html#ppu-modes behaves, there is a limit on how "big it can become" while being free to get as small as possible preserving the layout when seen from smaller viewports..

@ISSOtm
Copy link
Member Author

ISSOtm commented Jan 17, 2024

The same width limit applies to this diagram; though, perhaps what you're saying is that the proportions of the text versus the rest of the diagram are wrong? That I could fix.

Note that the PPU mode diagram becomes tiny on mobile screens, though; not that I believe anything can be done about that?

Screenshot of the aforementioned diagram rendered on a mobile screen

@avivace
Copy link
Member

avivace commented Jan 19, 2024

The same width limit applies to this diagram; though, perhaps what you're saying is that the proportions of the text versus the rest of the diagram are wrong? That I could fix.

Yep, I would say that should fix it

@ISSOtm
Copy link
Member Author

ISSOtm commented Jan 19, 2024

Then you can try playing with the proportions by adjusting font-size in the SVG's embedded CSS. I find the current design fine, so I'll leave that tweaking up to you; I'd just like a chance to review that before merging?

Copy link
Member

@avivace avivace left a comment

Choose a reason for hiding this comment

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

Alright, merging this in the current state as I don't have bandwidth for the suggested style changes, I'll come back at them later

@avivace avivace merged commit 5c8594f into gbdev:master Feb 1, 2024
2 checks passed
@ISSOtm ISSOtm deleted the sgb branch February 5, 2024 09:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
consistency - style Content format, text style, consistency in presenting the informations content Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants