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

Footer text nav has incorrect top margin #308

Closed
zdavis opened this Issue May 3, 2017 · 9 comments

Comments

Projects
None yet
3 participants
@zdavis
Member

zdavis commented May 3, 2017

screen shot 2017-05-03 at 1 05 17 pm

@zdavis

This comment has been minimized.

Show comment
Hide comment
@zdavis

zdavis May 3, 2017

Member

Right col should not have the top padding.

Member

zdavis commented May 3, 2017

Right col should not have the top padding.

@naomiyaki

This comment has been minimized.

Show comment
Hide comment
@naomiyaki

naomiyaki May 3, 2017

Member

Got it! Will fix when I get a spare moment.

Member

naomiyaki commented May 3, 2017

Got it! Will fix when I get a spare moment.

@naomiyaki

This comment has been minimized.

Show comment
Hide comment
@naomiyaki

naomiyaki May 4, 2017

Member

Has this been fixed? I'm not able to reproduce on Chrome or Firefox on any window size.

screen shot 2017-05-04 at 3 02 39 pm

Member

naomiyaki commented May 4, 2017

Has this been fixed? I'm not able to reproduce on Chrome or Firefox on any window size.

screen shot 2017-05-04 at 3 02 39 pm

@zdavis

This comment has been minimized.

Show comment
Hide comment
@zdavis

zdavis May 4, 2017

Member

It has not, AFAIK. It was an issue as of yesterday or the day before. Add another page record in the rails console p = Page.create(title: "test page, slug: "test" and see if you can reproduce. I can't remember exactly what fields are required on Pages, so if it doesn't save, check p.errors or p.valid? and fix whatever is wrong.

Member

zdavis commented May 4, 2017

It has not, AFAIK. It was an issue as of yesterday or the day before. Add another page record in the rails console p = Page.create(title: "test page, slug: "test" and see if you can reproduce. I can't remember exactly what fields are required on Pages, so if it doesn't save, check p.errors or p.valid? and fix whatever is wrong.

@naomiyaki

This comment has been minimized.

Show comment
Hide comment
@naomiyaki

naomiyaki May 5, 2017

Member

I did a bunch of tests with @SMaxOwok and discovered what I believe is the root of the issue. If you take a look at the DOM in your example above, there will likely be an empty <li> tag with a link to a page that isn’t showing a nav_title. The subsequent <li> has padding on top of it because of sibling spacing.

I was able to reproduce this repeatedly by creating a new page in the console without a nav_title:
p = Page.create(title: "Test Page", slug: "test", creator_id: "e126836d-1b19-4ca9-b942-870b64e0a3cc"). This will ad a valid page, and an empty

  • to the footer menu, even though show_in_footer and nav_title are nil on the page. This won’t break the layout the first time, but if you add two pages this way (with unique names and slugs), you should see this bug.
    manifold scholarship 2017-05-04 17-15-28

    We can of course address this bug in the view by hiding pages without nav_titles and show_in_footer in the view, but I wanted to run it by you first before addressing it in the Javascript as Max voiced that some of these fields should be setup as required on the model themselves. That said, if there are going to be pages without nav_titles, I will address that in the view, too! Please advise @zdavis .

  • Member

    naomiyaki commented May 5, 2017

    I did a bunch of tests with @SMaxOwok and discovered what I believe is the root of the issue. If you take a look at the DOM in your example above, there will likely be an empty <li> tag with a link to a page that isn’t showing a nav_title. The subsequent <li> has padding on top of it because of sibling spacing.

    I was able to reproduce this repeatedly by creating a new page in the console without a nav_title:
    p = Page.create(title: "Test Page", slug: "test", creator_id: "e126836d-1b19-4ca9-b942-870b64e0a3cc"). This will ad a valid page, and an empty

  • to the footer menu, even though show_in_footer and nav_title are nil on the page. This won’t break the layout the first time, but if you add two pages this way (with unique names and slugs), you should see this bug.
    manifold scholarship 2017-05-04 17-15-28

    We can of course address this bug in the view by hiding pages without nav_titles and show_in_footer in the view, but I wanted to run it by you first before addressing it in the Javascript as Max voiced that some of these fields should be setup as required on the model themselves. That said, if there are going to be pages without nav_titles, I will address that in the view, too! Please advise @zdavis .

  • @zdavis

    This comment has been minimized.

    Show comment
    Hide comment
    @zdavis

    zdavis May 5, 2017

    Member

    Ahh.. ideally, nav_title would be optional and title would be required. We'd show nav_title if set, and fallback to title if it's not set.

    Member

    zdavis commented May 5, 2017

    Ahh.. ideally, nav_title would be optional and title would be required. We'd show nav_title if set, and fallback to title if it's not set.

    @zdavis

    This comment has been minimized.

    Show comment
    Hide comment
    @zdavis

    zdavis May 5, 2017

    Member

    Thanks for digging into this one!

    Member

    zdavis commented May 5, 2017

    Thanks for digging into this one!

    naomiyaki added a commit that referenced this issue May 5, 2017

    [B] Require pages to have `show_in_footer` in view
    [Fixes #308]
    Also adds conditional fallback to title for page links without a
    `nav_title`

    @zdavis zdavis removed the master label May 8, 2017

    @zdavis zdavis added this to the v0.1.4 milestone May 8, 2017

    naomiyaki added a commit that referenced this issue May 11, 2017

    [B] Require pages to have `show_in_footer` in view
    [Fixes #308]
    Also adds conditional fallback to title for page links without a
    `nav_title`

    naomiyaki added a commit that referenced this issue May 11, 2017

    @zdavis

    This comment has been minimized.

    Show comment
    Hide comment
    @zdavis

    zdavis May 15, 2017

    Member

    This fix is present in the PR according to Naomi. It's lacking any tests, however, and I've asked @SMaxOwok to add them. Accordingly, I'm re-assigning this issue to Max.

    Member

    zdavis commented May 15, 2017

    This fix is present in the PR according to Naomi. It's lacking any tests, however, and I've asked @SMaxOwok to add them. Accordingly, I'm re-assigning this issue to Max.

    @zdavis zdavis assigned SMaxOwok and unassigned naomiyaki May 15, 2017

    @SMaxOwok

    This comment has been minimized.

    Show comment
    Hide comment
    @SMaxOwok

    SMaxOwok May 15, 2017

    Member

    Test added
    f95b85d

    Member

    SMaxOwok commented May 15, 2017

    Test added
    f95b85d

    SMaxOwok added a commit that referenced this issue May 17, 2017

    [B] Require pages to have `show_in_footer` in view
    [Fixes #308]
    Also adds conditional fallback to title for page links without a
    `nav_title`

    SMaxOwok added a commit that referenced this issue May 17, 2017

    @zdavis zdavis closed this in 1df26d7 May 17, 2017

    zdavis added a commit that referenced this issue May 17, 2017

    @zdavis zdavis removed the in progress label Oct 2, 2017

    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment