| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
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
| Original file line number | Diff line number | Diff line change | |||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|
| Expand Up | @@ -4,6 +4,7 @@ body { | ||||||||||
| background-image: linear-gradient(#d7cdc0ff, #fdf7f6ff); | |||||||||||
| margin-right: 10%; | |||||||||||
| margin-left: 10%; | |||||||||||
| overflow-wrap: break-word; | |||||||||||
| } | |||||||||||
| .block { | |||||||||||
| background-color: rgb(247, 243, 239); | |||||||||||
| Expand All | @@ -13,19 +14,36 @@ body { | ||||||||||
| padding-bottom: .1em; | |||||||||||
| border-radius: 2em; | |||||||||||
| } | |||||||||||
| img { | |||||||||||
| max-width: 100%; | |||||||||||
| height: auto; | |||||||||||
| } | |||||||||||
| .banner { | |||||||||||
| text-align: center; | |||||||||||
| font-weight: bold; | |||||||||||
| td img { | |||||||||||
| height: 150px; | |||||||||||
| } | |||||||||||
| } | |||||||||||
| big { | |||||||||||
| font-size: 400%; | |||||||||||
| } | |||||||||||
| small { | |||||||||||
| font-size: 70%; | |||||||||||
| } | |||||||||||
| pre { | |||||||||||
| background-color: rgb(247, 243, 239); | |||||||||||
| border: 1px solid #c5bbb0; | |||||||||||
| border-radius: 0.5em; | |||||||||||
| padding: 1em; | |||||||||||
| overflow-x: auto; | |||||||||||
| } | |||||||||||
| code { | |||||||||||
| background-color: rgb(247, 243, 239); | |||||||||||
| padding: 0.1em 0.3em; | |||||||||||
| border-radius: 0.25em; | |||||||||||
| } | |||||||||||
| pre code { | |||||||||||
| background-color: transparent; | |||||||||||
| padding: 0; | |||||||||||
| } | |||||||||||
|
Comment thread
Comment on lines
+31
to
+46
Copy link
Copy Markdown
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low QualityI would really like to learn from this. To that end, would you mind extending the commit message quite a bit? I'd like it more in line with https://github.blog/2022-06-30-write-better-commits-build-better-projects/, in particular with a strong focus on this part:
Sorry, something went wrong.
All reactions
Copy link
Copy Markdown
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low QualitySure, I appreciate your enthusiasm for this update, I'll provide a more comprehensive and clear set of commit msgs.
Sorry, something went wrong.
All reactions
|
|||||||||||
| div.feature-matrix { | |||||||||||
| table { | |||||||||||
| border-collapse: collapse; | |||||||||||
| Expand Down Expand Up | @@ -60,7 +78,16 @@ div.feature-matrix { | ||||||||||
| vertical-align: center; | |||||||||||
| } | |||||||||||
| } | |||||||||||
| @media screen and (max-width: 450px) { | |||||||||||
| big { | |||||||||||
| font-size: 300%; | |||||||||||
| } | |||||||||||
| } | |||||||||||
| @media screen and (max-width: 767px) { | |||||||||||
| body { | |||||||||||
| margin-right: 3%; | |||||||||||
| margin-left: 3%; | |||||||||||
| } | |||||||||||
| .tg { | |||||||||||
| width: auto !important; | |||||||||||
| } | |||||||||||
| Expand Down | |||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| Expand Up | @@ -7,21 +7,6 @@ | |
| <small>... one iteration at a time... </small> | ||
| {{- end -}} | ||
| <div class="banner"> | ||
| {{- if eq .Kind "home" }} | ||
|
Comment thread
Copy link
Copy Markdown
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality
Sorry, something went wrong.
All reactions
Copy link
Copy Markdown
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low QualityI admit, this was no regression, it was intentional. As a user navigating from the homepage to the other non-homepages on desktop, I got the impression that the banners weren't rendering properly, they didn't look right compared to the homepage banner. I was honestly unsure if the table banner layout itself had a bug causing it to look different than the homepage's on both desktop and mobile. Given how infrequently this repo is updated, instead of potentially waiting a while for an answer before pushing, I just lead with the assumption I was fixing a bug and that someone would tell me otherwise if not! So here's my case presented to you for changing it and why it lands in scope to affect desktop.
Generally, it's desirable to have the logo and other brand elements appear consistently across similar contexts to enhance recognition. The pages all share a similar body structure and background and they position the banner in the same area, so the banners are basically serving the same purpose. The visual concept behind the two banner types is basically the same: Because the concept is the same but the implementation is different enough within the same context, it reads as inconsistent rather than intentional. It doesn't really look like the branding banner is being experienced in a new way, it looks like either a bug is rendering it differently than the homepage or a decision couldn't be made on how the banner looked best so both variants were included.
Cells don't stack or reflow, and it's harder to control how images scale within them without fighting the table structure. The banner table already looked inconsistent with the homepage's banner on desktop, so rather than fighting to make it work responsively, it made more sense to just remove it, solving both the branding inconsistency affecting desktop and the mobile issues in one move. I updated my commit message to make clear that this is a deliberate unification of page banner layouts. Though I recommend against it, if you would prefer me to restore the table structure layout for the non-homepages, I can do that and try to figure out a way to get it to behave properly on mobile. Let me know if that's what you would prefer instead.
Sorry, something went wrong.
All reactions
Copy link
Copy Markdown
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low QualityThank you for taking the time to explain the rationale. Explained like I am 5, this actually makes total sense to me. Thank you for indulging me!
Sorry, something went wrong.
All reactions
|
||
| {{ partial "logo.html" . }} | ||
| {{ partial "subtitle.html" . }} | ||
| {{- else }} | ||
| <table style="width: 100%"> | ||
| <tbody> | ||
| <tr> | ||
| <td> | ||
| {{ partial "logo.html" . }} | ||
| </td> | ||
| <td> | ||
| {{ partial "subtitle.html" . }} | ||
| </td> | ||
| </tr> | ||
| </tbody> | ||
| </table> | ||
| {{- end }} | ||
| </div>{{ if eq .Kind "home" }}<hr />{{ end }} | ||
| Back | FazBrowse Home | New Git URL |
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.
Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low QualityIs this a fixup for dad29bc that would want to be squashed into that commit instead of living on its own? Also, the commit message does not help me understand the problem, the context, and why the proposed solution is the best way to solve the problem. I would love it to help me, though.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
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.
Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low QualityThose are scoped separately, I will update the commit msg to make that clear.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.