tinacms / tinacms

A fully open-source headless CMS that supports Markdown and Visual Editing
https://tina.io
Apache License 2.0
11.32k stars 573 forks source link

Fixed sticky menu on inline editing #528

Closed weibenfalk closed 4 years ago

weibenfalk commented 4 years ago

Hey.

This may seem a little much for this functionality. But check it out. I think it's necessary as the structure on things are now. It isn't possible to just set the positioning to "sticky"

Thomas

spbyrne commented 4 years ago

Hey @weibenfalk, this looks great! Testing locally and I like the behaviour.

I started playing around with the code locally to address a few things:

  1. Throttle the scroll event; not sure what a good level of throttling is for this kind of thing.
  2. When the menu reaches the bottom of the WYSIWYG it should be un-stuck.
  3. There should be a placeholder to 'hold' the space of the toolbar when it's fixed, so that the moment it becomes fixed things don't move around.

For 3 I thought maybe having a wrapper that's position relative, making the placeholder always there, and then toggling the toolbar itself between position absolute & fixed could work. Here's roughly what I was doing to experiment with that approach:

const MenuContainer = styled.div`
  display: flex;
  justify-content: space-between;
  position: absolute;
  top: 0;
  width: 100%;
  max-width: 100%;
  background-color: white;
  border-radius: ${radius()};
  box-shadow: 0px 2px 3px rgba(0, 0, 0, 0.12);
  border: 1px solid ${color.grey(2)};
  overflow: visible;
  display: flex;
  flex: 0 0 auto;
  z-index: 10;
  margin: 0 0 0.75rem 0;
`

const MeneuPlaceholder = styled.div`
  position: relative;
  opacity: 0;
  visibility: hidden;
  pointer-events: none;
  user-select: none;
`

const MenuWrapper = styled(
  ({ menuFixed, menuWidth, menuRef, children, ...styleProps }) => {
    return (
      <div ref={menuRef} {...styleProps}>
        <MenuContainer>{children}</MenuContainer>
        <MeneuPlaceholder>Test</MeneuPlaceholder>
      </div>
    )
  }
)`
  position: relative;

  ${MenuContainer}, ${MeneuPlaceholder} {
    max-width: ${({ menuWidth }) => `${menuWidth}px`};
  }

  ${props =>
    props.menuFixed &&
    css`
      ${MenuContainer} {
        position: fixed;
      }
    `}
`

Unfortunately I don't have time to continue this right now as we're looking to get the improved blocks UI out ASAP. If you want to look at these changes let me know! Otherwise I can return to this once the blocks UI is shipped.

👍

weibenfalk commented 4 years ago

Hey.

I hope I got time to check this out myself … A lot of stuff happening now before Christmas and all. Just completed another issue for TinaCMS also … =)

Can’t promise anything right now though … will try to squeeze it in between other things to do.

Is throttling needed? Is it such a performance issue as this is only active when the inline editor is open?

Best, Thomas

weibenfalk commented 4 years ago

@spbyrne I’ve played around a little with throttling now. Built my own simple function for that. But … it is really ”clunky” even when the timeout is set to only 100 … it misses to stick the menu some times.

Also one thought. This thing is kind of already throttled. The callback function, when scrolling, isn’t doing anything if we’re not scrolled to the top or if we scroll away from the top. That is; Setting the menu to stick or non stick. Otherwise it just do nothing. So can’t be a performance issue?

So it’s not a function that triggers something on every scroll iteration … It’s almost kind of the same as having a throttling function that will call the callback in the given time interval. The throttling function will also run on each iteration but will only call the callback on the given timeout time …

… Am I doing some wrong thinking here or what? =)

Thomas

weibenfalk commented 4 years ago

@spbyrne Hey ... I've fixed nr 2 in the list now. Just pushed it. Starting to get a lot of conditional checks now ;)

weibenfalk commented 4 years ago

@spbyrne, Thought of one more thing now. What happens if the webpage itself has a fixed header? Then this menu will be positioned on top of that. Covering it. That's no good ... Maybe it would be good also having an option to set the offset in pixels from the top for the absolute positioned toolbar?

Otherwise I think the divs need to be restructured so the toolbar is inside of the div that holds the content. And then do some calculations to fix it inside of that one ... but haven't thought it true so have no clue right now on how to do it. Probably need the offset option anyways.

Thoughts?

spbyrne commented 4 years ago

It sounds like throttling is less important than I speculated, that's okay. You can make the call on that. It's not an expensive operation happening on scroll so I was probably wrong to think there would be a noticeable performance impact.

Fixed headers are something to consider, but a perfect solution may be too complex. I think a configurable offset is the simplest solution that would work well for most situations, defaulting to 0.

This is looking really good and I can't wait to see it paired with the new blocks UI!

weibenfalk commented 4 years ago

Great!

You probably saw that I fixed your second demand. The third I don’t really get … everything looks fine here and don’t break. =) As you have started on that one maybe it’s better for you to finish it so you get it as you want it to be?

One thing also … had to comment out the block in the graphQL query to get the Gatsby site to work. It worked yesterday but not today telling me it can’t find it in the query … you know anything about this issue when developing?

17 dec. 2019 kl. 14:30 skrev Scott Byrne notifications@github.com:

It sounds like throttling is less important than I speculated, that's okay. You can make the call on that. It's not an expensive operation happening on scroll so I was probably wrong to think there would be a noticeable performance impact.

Fixed headers are something to consider, but a perfect solution may be too complex. I think a configurable offset is the simplest solution that would work well for most situations, defaulting to 0.

This is looking really good and I can't wait to see it paired with the new blocks UI!

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/tinacms/tinacms/pull/528?email_source=notifications&email_token=ACVRL3RZWCLN4DJIHJPSDQLQZDIAFA5CNFSM4JZ6NSS2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEHCL5AQ#issuecomment-566541954, or unsubscribe https://github.com/notifications/unsubscribe-auth/ACVRL3VZ2LFK4S7H5K5QPV3QZDIAFANCNFSM4JZ6NSSQ.

spbyrne commented 4 years ago

Not sure about that blocks issue. Often when I encounter a GQL issue I reset my content folder locally to see if that fixes it. Otherwise I'll test it out when merging to see what's up.

My feedback was more nitpicky than anything; I think this looks good to merge! 😄Really appreciate your help with this.

spbyrne commented 4 years ago

Maybe try merging master to see if that helps with your issue?

weibenfalk commented 4 years ago

The things is that it works even when I comment out the blocks :) what exactly is that for? Just getting to know the code ...

Skickat från min iPhone

17 dec. 2019 kl. 14:58 skrev Scott Byrne notifications@github.com:

 Maybe try merging master to see if that helps with your issue?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or unsubscribe.

spbyrne commented 4 years ago

I can get it running locally but the sticky header is no longer working 😦 I think it may be an issue on my end, I'm just not sure.

I think your GraphQL issue was from the block with an image being removed. Since there's no block with an image, the query fails at build. If you comment out the block itself, the query is still there. In Grande I have dummy.json files to get around this issue. This way if no post has an image but you query for an image you still get something (from the dummy file) so the query doesn't fail.

The blog blocks component (<BlogBlocks form={form} data={blocks} />) is a custom component for the demo, but it's just a wrapper for the tina InlineBlocks component; you can see the setup in blog-blocks.js.

InlineBlocks takes an array of blocks data and a list of components and renders out each block, but I'm not the best person to explain exactly what's going on. DJ wrote a great post that you can take a look at on our blog..

weibenfalk commented 4 years ago

Hmmm … should work. It was working when I pushed it. Got time to do a quick check … it’s working flawless on my end and it is the updated code that has been pushed.

Thanks! … Yes I removed the image there so that must be it. No image in the markdown file then. =)

weibenfalk commented 4 years ago

Did you get it to work @spbyrne ?

kendallstrautman commented 4 years ago

Possibly fixes #425

spbyrne commented 4 years ago

Maybe we should take the same approach as the color picker and first merge to a branch on this repo? That would make things a bit easier. In that case I can approve the changes now.

ncphillips commented 4 years ago

I changed the base to sticky-menu. The conflicting files will have to be fixed first though. Please accept the incoming changes fro the package-lock.json files.

weibenfalk commented 4 years ago

Is there something I have to do on this?

spbyrne commented 4 years ago

@weibenfalk If you could accept any incoming changes and resolve those conflicts we can get this merged in 👍 I was going to do that this morning but it looks like it will require changes on your end.

weibenfalk commented 4 years ago

I think I've solved it now. Got a little confused as this is still on my fork .. then this new branch sticky-menu ... So .. hope everything is like it should.

weibenfalk commented 4 years ago

@spbyrne @ncphillips Did this get merged or what happened? Didn't get a notice about it =)

ncphillips commented 4 years ago

Yes it did :) There might be a bug though. See #618