Develop landing page landing page header #121 #126
Conversation
|
For #121 |
| } | ||
| app-drawer ul { | ||
| text-align: left !important; | ||
| } | ||
| app-header-layout { | ||
| height: 100vh; | ||
| height: 200vh; |
There was a problem hiding this comment.
Hmm, I'll need to check this out in practice, but it looks strange to essentially be 200% of the view height. Maybe I'm wrong, but I'll check this out when testing.
There was a problem hiding this comment.
This is a good start, but there are still plenty of differences between the comps and the way this looks. Here are some of the bigger things I saw:
- The text size and margin sizes appear to be too small
- The menu button shows up for the desktop layout (it should only show up for the mobile layout)
- The selected text is bold black instead of green like the comp shows
- The layout doesn't appear to be themed for mobile at all (there are other things on the landing page which have a different mobile layout via CSS based on the screen width)
- There appears to be a regression where there are now 2 vertical scroll bars, probably because of that 200vh
|
ok I will work on it again, I spent two hours trying to figure out how to
change the selected text color to black to no avail, but I will try again.
…On Wed, Sep 18, 2019 at 11:06 PM Chris Schmitz ***@***.***> wrote:
***@***.**** requested changes on this pull request.
This is a good start, but there are still plenty of differences between
the comps and the way this looks. Here are some of the bigger things I saw:
- The text size and margin sizes appear to be too small
- The menu button shows up for the desktop layout (it should only show
up for the mobile layout)
- The selected text is bold black instead of green like the comp shows
- The layout doesn't appear to be themed for mobile at all (there are
other things on the landing page which have a different mobile layout via
CSS based on the screen width)
- There appears to be a regression where there are now 2 vertical
scroll bars, probably because of that 200vh
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#126?email_source=notifications&email_token=AECPVSPP2THWEQAKXT6YRJ3QKMJCJA5CNFSM4IWYUO22YY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCFHDM3I#pullrequestreview-290338413>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AECPVSKGVRNMCEQFRY2EUTLQKMJCJANCNFSM4IWYUO2Q>
.
|
|
I mean green
…On Thu, Sep 19, 2019 at 3:23 PM Kyle H ***@***.***> wrote:
ok I will work on it again, I spent two hours trying to figure out how to
change the selected text color to black to no avail, but I will try again.
On Wed, Sep 18, 2019 at 11:06 PM Chris Schmitz ***@***.***>
wrote:
> ***@***.**** requested changes on this pull request.
>
> This is a good start, but there are still plenty of differences between
> the comps and the way this looks. Here are some of the bigger things I saw:
>
> - The text size and margin sizes appear to be too small
> - The menu button shows up for the desktop layout (it should only
> show up for the mobile layout)
> - The selected text is bold black instead of green like the comp shows
> - The layout doesn't appear to be themed for mobile at all (there are
> other things on the landing page which have a different mobile layout via
> CSS based on the screen width)
> - There appears to be a regression where there are now 2 vertical
> scroll bars, probably because of that 200vh
>
> —
> You are receiving this because you authored the thread.
> Reply to this email directly, view it on GitHub
> <#126?email_source=notifications&email_token=AECPVSPP2THWEQAKXT6YRJ3QKMJCJA5CNFSM4IWYUO22YY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCFHDM3I#pullrequestreview-290338413>,
> or mute the thread
> <https://github.com/notifications/unsubscribe-auth/AECPVSKGVRNMCEQFRY2EUTLQKMJCJANCNFSM4IWYUO2Q>
> .
>
|
|
If you can't figure something out you can just note that the implementation isn't complete and we can note it in the ticket and merge what we have so far. At least it will be closer to correct and we can continue working to try and resolve any lingering differences. As far as the specific issue, I would first check if I could update the style by inspecting the CSS in the browser and changing it there (sometimes I need to mark a style as |
|
Is this something you're planning to work on during the hackathon this Saturday? |
|
Merging, since progress has stalled, so we have the changes so far. Work will continue on #121. |
|
This caused regressions, so reversing this merge. |
Develop landing page page header.