Skip to content
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

Allow code editor to be resized #729

Merged
merged 6 commits into from
Jul 13, 2020

Conversation

vabarbosa
Copy link
Contributor

@vabarbosa vabarbosa commented Jul 9, 2020

Updated the code editor component in the code snippet sidebar and the code editor component in the metadata editor form

  • code editor component can be scrolled using the scrollbars
  • code editor component opens at an appropriate default size so other components do not immediately get pushed off screen

Fixes #728

Before:

image

After:

image

Developer's Certificate of Origin 1.1

   By making a contribution to this project, I certify that:

   (a) The contribution was created in whole or in part by me and I
       have the right to submit it under the Apache License 2.0; or

   (b) The contribution is based upon previous work that, to the best
       of my knowledge, is covered under an appropriate open source
       license and I have the right under that license to submit that
       work with modifications, whether created in whole or in part
       by me, under the same open source license (unless I am
       permitted to submit under a different license), as indicated
       in the file; or

   (c) The contribution was provided directly to me by some other
       person who certified (a), (b) or (c) and I have not modified
       it.

   (d) I understand and agree that this project and the contribution
       are public and that a record of the contribution (including all
       personal information I submit with it, including my sign-off) is
       maintained indefinitely and may be redistributed consistent with
       this project or the open source license(s) involved.

Update the code editor components so they can be resized
@vabarbosa vabarbosa added kind:bug Something isn't working area:front-end labels Jul 9, 2020
@vabarbosa vabarbosa self-assigned this Jul 9, 2020
@vabarbosa vabarbosa added the component:metadata-editor Metadata Editor Extension label Jul 9, 2020
Copy link
Member

@lresende lresende left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. I was really looking forward to having more than one line out of the box when creating new code-snippets.

@ajbozarth
Copy link
Member

Two points on this:

  1. Does this address the issue from this comment on the original PR? Create metadata editor #589 (review) @marthacryan and I could never recreate it, but checking the css the actual code editor is still only 1 line despite the Dom element having a min height of 100px. I would argue that the scrollability shouldn't require a min height.

  2. This only kinda solve the issue as you raised it in Code Editor components are not scrollable #728 which is that the metadata editor widget doesn't scroll. If I shrink my window size vertically enough the save button will still go off screen. Though this fix is still important, we should make sure the widget itself can scroll as well since we don't control the quantity of metadata fields that may be displayed by other namespaces.

@vabarbosa
Copy link
Contributor Author

vabarbosa commented Jul 9, 2020

Two points on this:

1. Does this address the issue from this comment on the original PR? [#589 (review)](https://github.com/elyra-ai/elyra/pull/589#pullrequestreview-433532524) @marthacryan and I could never recreate it, 

i used to run into that issue a couple times early on but i have not seen it after the past couple commits to the metadata editor. i tried again now and not reproducing so while it looks like it is no longer happening i can't say for certain this update addressed it.

but checking the css the actual code editor is still only 1 line despite the Dom element having a min height of 100px. I would argue that the scrollability shouldn't require a min height.

the min-height is there just so when you open a new editor window you have a box that is clearly visible but if that is not desired it can be removed.

2. This only kinda solve the issue as you raised it in #728 which is that the metadata editor widget doesn't scroll. If I shrink my window size vertically enough the save button will still go off screen. Though this fix is still important, we should make sure the widget itself can scroll as well since we don't control the quantity of metadata fields that may be displayed by other namespaces.

just committed an updated which should also allow the widget itself doesn't scroll. you should now be able to scroll if your window is made small enough

image

Copy link
Member

@marthacryan marthacryan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just tried this and it worked well for me, good catch! Since we had that bug related to adding a min-height (maybe @karlaspuldaro or @kevin-bates could try it out and see if they get the same bug) it makes me a little nervous to add the min-height (but if you were experiencing that and aren't anymore maybe it's fine). Anyway, I agree with Luciano that it's better with a min-height and if the bug doesn't show up for people that had it before, I think we should keep it in. Thanks!

@ajbozarth
Copy link
Member

Thanks for the quick updates, I just tested this locally and it works great for new snippet editor but it seems to have a weird bug when editing. The code box is not resizable when editing an existing snippet, but if you shrink the window size till the newly added widget scrolling appears the resize corn shows up (but is of course not resizable since the window is too small)
Screen Shot 2020-07-09 at 3 07 53 PM
Screen Shot 2020-07-09 at 3 08 01 PM

@vabarbosa
Copy link
Contributor Author

Thanks for the quick updates, I just tested this locally and it works great for new snippet editor but it seems to have a weird bug when editing. The code box is not resizable when editing an existing snippet, but if you shrink the window size till the newly added widget scrolling appears the resize corn shows up (but is of course not resizable since the window is too small)

hmmm, i am not seeing that. which browser are you using? out of curiosity, if you put focus out and then back into the code editor does it re-appear?

@ajbozarth
Copy link
Member

I just checked and it's a bug unique to Safari (it works fine in FF and Chrome)

Unverified

This commit is not signed, but one or more authors requires that any commit attributed to them is signed.
Per review comment resize fails  on Safari when opening a new editor form

Unverified

This commit is not signed, but one or more authors requires that any commit attributed to them is signed.
@vabarbosa
Copy link
Contributor Author

vabarbosa commented Jul 10, 2020

I just checked and it's a bug unique to Safari (it works fine in FF and Chrome)

@ajbozarth the resize issue in Safari should be addressed now

Copy link
Member

@ajbozarth ajbozarth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So your recent update has fixed the ability for Safari to resize, but now I'm seeing some new oddities:

  1. In Safari when you mouse over the resize corner the mouse pointer is a text cursor (just like the rest of the editor), whereas in chrome and FF its the resize cursor.

  2. In FF and Chrome the resize no longer respects the minimum height whereas Safari does, I don't really have an opinion on which is better but they should be consistent with each other.

If these oddities don't seem fixable without hacky code I'm willing to let them go and approve this, but I'd rather see them fixed if there's a clean solution

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
Update the cursor on Safari when hovering over the resize controller

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
@vabarbosa
Copy link
Contributor Author

1. In Safari when you mouse over the resize corner the mouse pointer is a text cursor (just like the rest of the editor), whereas in chrome and FF its the resize cursor.

all set. Safari should no longer display the text cursor. however it default to the regular cursor while Firefox/Chrome display the resize cursor.

2. In FF and Chrome the resize no longer respects the minimum height whereas Safari does, I don't really have an opinion on which is better but they should be consistent with each other.

when using CSS resize Safari treats initial element height as a minimum height (Chrome, Firefox do not). i initially had min-height configured and all three behaved similar but there were previous comments about preferring not setting a min-height. in any case, i have added the min-height back so there is consistent behavior across browsers.

@vabarbosa vabarbosa requested a review from ajbozarth July 11, 2020 01:32
Copy link
Member

@karlaspuldaro karlaspuldaro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tried this out in chrome, ff and safari, works great :)

@ajbozarth
Copy link
Member

@vabarbosa thanks for the fixes, but now I'm no longer seeing the original feature:

code editor component opens at an appropriate default size so other components do not immediately get pushed off screen

On both Chrome and Safari it always starts at the min height now, no matter the content

@lresende lresende merged commit 1718357 into elyra-ai:master Jul 13, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:front-end component:metadata-editor Metadata Editor Extension kind:bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Code Editor components are not scrollable
5 participants