accordproject / web-components

React Components for Accord Project
Apache License 2.0
120 stars 101 forks source link

style(ui-markdown-editor): resolve toolbar icon shrinkage - I228 #229

Closed K-Kumar-01 closed 3 years ago

K-Kumar-01 commented 3 years ago

Closes #228

The toolbar icons in markdown shrank too much. Resolved that issue.

Changes

Flags

Screenshots or Video

DeepinScreenshot_20201220144521 DeepinScreenshot_20201220144528 DeepinScreenshot_20201220144537 DeepinScreenshot_20201220144544 DeepinScreenshot_20201220144610

Author Checklist

jolanglinais commented 3 years ago

We should have addressed this in #228, but any thoughts on how this should ideally be @Michael-Grover? My first thought is having a minimum width for the whole editor, so the icons can not need to resize or anything.

K-Kumar-01 commented 3 years ago

We should have addressed this in #228, but any thoughts on how this should ideally be @Michael-Grover? My first thought is having a minimum width for the whole editor, so the icons can not need to resize or anything.

@irmerk The icons will shrink when the window is resized so I don't think this will be a good approach. I don't have any other idea about how this should be tackled though

Michael-Grover commented 3 years ago

@irmerk I agree that a minimum width for the editor would make sense. @K-Kumar-01 How about making the minimum the same width as the contract editor component? It looks like that's 670px.

K-Kumar-01 commented 3 years ago

@irmerk I agree that a minimum width for the editor would make sense. @K-Kumar-01 How about making the minimum the same width as the contract editor component? It looks like that's 670px.

@Michael-Grover
I cannot understand what to do. Can you explain please as I am confused about what to do? :sweat_smile:

Michael-Grover commented 3 years ago

@K-Kumar-01 I'm not sure what the best way to do this is from a coding perspective, but what I'm suggesting is that we:

Make this sure this component can't reduce to a width smaller than 670px. When the screen is too narrow to accommodate the 670px width component, the component will overflow off the right side of the screen. For example, if the viewport is 400px width, the component will be 670px width, however if the viewport is 1200px width, the component will grow in size to fit the screen like it currently does. This will ensure the toolbar is never so narrow that the icons shrink.

image

K-Kumar-01 commented 3 years ago

@K-Kumar-01 I'm not sure what the best way to do this is from a coding perspective, but what I'm suggesting is that we:

Make this sure this component can't reduce to a width smaller than 670px. When the screen is too narrow to accommodate the 670px width component, the component will overflow off the right side of the screen. For example, if the viewport is 400px width, the component will be 670px width, however if the viewport is 1200px width, the component will grow in size to fit the screen like it currently does. This will ensure the toolbar is never so narrow that the icons shrink.

image

@Michael-Grover Well, thanks for the explanation. I had a few doubts and queries. On mobile screens wouldn't it be uncomfortable? Besides, I think that it may cause some styling problems though I am not sure about it. I would like to draw attention to one more thing that 670px +210px i.e. a total of 880px or greater would be required for the working when the sidebar is shown. So I am not fully confident about this approach. Would like suggestions from @irmerk also on how can we go.

jolanglinais commented 3 years ago

I don't think we should be concerned with mobile usage. This repository is not geared towards that end. In the future, we can approach mobile functionality.

In the meantime, I think we should set a minimum width for the editor and toolbar and let if overflow if the page width is reduced to less than that. This should remove the issue of icons resizing because nothing can go lower in width than what we specify.

K-Kumar-01 commented 3 years ago

I don't think we should be concerned with mobile usage. This repository is not geared towards that end. In the future, we can approach mobile functionality.

In the meantime, I think we should set a minimum width for the editor and toolbar and let if overflow if the page width is reduced to less than that. This should remove the issue of icons resizing because nothing can go lower in width than what we specify.

@irmerk @Michael-Grover I have tried implementing the above.

Attaching screenshots for reference.

DeepinScreenshot_20201222172038 DeepinScreenshot_20201222172105

If it is suitable, I will commit it otherwise make any more changes needed. I used js for styling as the styles applied were to the body tag and a slate component which I could not figure out to apply. The styles though will be applied only here using useEffect.

Code

useEffect(()=>{
    const toolbar = document.getElementById("ap-rich-text-editor-toolbar");
    toolbar.parentElement.style.minWidth = "630px";
    document.body.style.overflowX = "auto";
  },[])

Let me know if there are any changes required.

Michael-Grover commented 3 years ago

Hi @K-Kumar-01 , I opened the Netlify link and I'm somehow still able to reduce the width of the component below the minimum we set. @irmerk are you seeing the same behavior?

image

jolanglinais commented 3 years ago

@K-Kumar-01 could you add your commits to the PR so we can view the changes?

K-Kumar-01 commented 3 years ago

@irmerk @Michael-Grover I have added my latest commits to the PR. Let me know if there are any changes required. I apologize for the earlier incident @Michael-Grover. I forgot to push the commits :sweat_smile:

K-Kumar-01 commented 3 years ago

@irmerk

I. think the consumer of the ui-markdown-editor component should determine the overflow. In this instance that would be the Storybook.

I am not quite getting what you are trying to say. :sweat_smile: Could you please tell me about this?

As for the useEffect part, the minWidth only makes the toolbar to a min-width. The area that will contain the text (where the slate tag is used) won't be shrinked. So I had to resort to useEffect for that part

K-Kumar-01 commented 3 years ago

@irmerk

I. think the consumer of the ui-markdown-editor component should determine the overflow. In this instance that would be the Storybook.

I am not quite getting what you are trying to say. Could you please tell me about this?

As for the useEffect part, the minWidth only makes the toolbar to a min-width. The area that will contain the text (where the slate tag is used) won't be shrinked. So I had to resort to useEffect for that part

@irmerk Any updates on this one?

jolanglinais commented 3 years ago

Yes the ui-markdown-editor is a component, which is used in the Storybook and ui-contract-editor, or any other app that decides to use it. Those components are the ones that should decide the overflow of ui-markdown-editor when they render it.

K-Kumar-01 commented 3 years ago

Yes the ui-markdown-editor is a component, which is used in the Storybook and ui-contract-editor, or any other app that decides to use it. Those components are the ones that should decide the overflow of ui-markdown-editor when they render it.

@irmerk So basically the component which is currently active i.e. ui-markdown-editor or ui-contract-editor should be responsible for the resize? Am I correct here?

jolanglinais commented 3 years ago

Yes I believe that is correct.

K-Kumar-01 commented 3 years ago

Yes I believe that is correct.

@irmerk @Michael-Grover I have changed the approach as suggested by @irmerk . There are global body styles added for the storybook to see overflow and div added only for markdown editor. Let me know if there are any changes required :)

jolanglinais commented 3 years ago

This still doesn't look like the right approach to me - wrapping the editor in a div with a min-width. I'll look into this more when I have a chance.

K-Kumar-01 commented 3 years ago

Sorry I haven't had much time to look into this more before and haven't been able to give very helpful guidance. I didn't look at this well enough and that resulted in this idea.

But I believe all we really need is to configure min-width: 600px in these two places. We don't need overflow-x: auto or anything else.

@irmerk If overflow-x:auto is not set then on resizing window below a certain size the content gets cut off in my implementations. I will try to implement the suggested changes but I think that overflow-x:auto would be required. Will confirm and tell you after the implementation.

jolanglinais commented 3 years ago

Ah, well that should be added in the storybook, not on ui-markdown-editor

K-Kumar-01 commented 3 years ago

@irmerk and @Michael-Grover I have made some changes to the PR. Basically providing min-width to toolbar menu and text editor separately. Let me know if there are any changes required.