Skip to content

Conversation

@mfikria
Copy link
Contributor

@mfikria mfikria commented Jul 4, 2019

No description provided.

Copy link
Collaborator

@maxceem maxceem left a comment

Choose a reason for hiding this comment

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

Hi @mfikria

It looks like the issue is not fixed and the field still doesn't expand to show the whole text:

image

Also, if I resize the text field manually and after type "Enter" the field shrinks again and truncates some text.

Also, every time I type in the texture there is an error in the browser console:

image

By the way, do the changes you did fix any case?

@mfikria
Copy link
Contributor Author

mfikria commented Jul 6, 2019

@maxceem
Copy link
Collaborator

maxceem commented Jul 6, 2019

@mfikria yes, I used a new version from react-components, though your video looks good, I'll try to test one more time tomorrow.

@maxceem
Copy link
Collaborator

maxceem commented Jul 7, 2019

@mfikria I've tested one more time and made sure that I use the updated code (I've added a comment to your new code and I can see it browser console), but it works with issues for me:

  1. When I load page there is an error in the browser console:

    image

  2. When I press Enter there is another error in the console:

    image

  3. and after 2-3 times press Enter the text is truncated again, see video https://monosnap.com/file/1YL4YJgXWPaKtBI0fC0DN593zr7C3m

OS: macOS 10.14.5
Browser: Google Chrome Version 75.0.3770.100 (Official Build) (64-bit)
Also tested in: Firefox 67.0.4

@mfikria
Copy link
Contributor Author

mfikria commented Jul 8, 2019

@mfikria
Copy link
Contributor Author

mfikria commented Jul 8, 2019

@maxceem about the Invariant Violation error i think it is because the missmatch version of react/react-dom package between connect-app and appirio-tech-react-components.
See explanation in https://gist.github.com/jimfb/4faa6cbfb1ef476bd105

Note that this error happened in local development that use npm link
So to fix the issue follow these steps:

  1. in connect-app folder project: remove node_modules, revert any change in package.json and package-lock.json
  2. in appirio-tech/react-components folder project: run npm link, remove node_modules, revert any change in package.json and package-lock.json
  3. in connect-app folder project: run npm link appirio-tech-react-components, run npm i
  4. run connect-app via npm start

Copy link
Collaborator

@maxceem maxceem left a comment

Choose a reason for hiding this comment

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

Great, @mfikria works good now.

I didn't manage to run connect with react-components using your guide in some reason, so I've just copied the code of updated react-components repo to the node_modules of Connect app and it works.

There are only a few questions regarding the code:

  1. Do we need changes made in the Connect app? it feels that they don't do anything as function handleAccordionOpen(accordion, index) {} doesn't have a code.

  2. In the react-components you've added a new prop minRows though we already have an existent prop rows which suppose to set the default rows quantity. Can we use the existent rows prop, or there is some reason we have to introduce a new one, see https://monosnap.com/file/7835nxJZQShmyX2Q2VxMV8laXiEfoX

@mfikria
Copy link
Contributor Author

mfikria commented Jul 8, 2019

  1. handleAccordionOpen is called when accordion is opened. Since I did not know a way to call textarea.focus() on that function. The changes from this pr can be excluded except change to file src/projects/detail/components/SpecSection.scss which made the textarea can be resized manually.

  2. Since we depend on https://www.npmjs.com/package/react-textarea-autosize. There is a prop called minRows and it is different from the native rows https://www.w3schools.com/tags/att_textarea_rows.asp

@maxceem
Copy link
Collaborator

maxceem commented Jul 8, 2019

@mfikria

  1. ok, maybe we remove it, after deciding what to do with [$150] Make text fields expandable in Scope form #3128 (comment)

  2. Since we depend on https://www.npmjs.com/package/react-textarea-autosize. There is a prop called minRows and it is different from the native rows https://www.w3schools.com/tags/att_textarea_rows.asp

Got it. I guess, then as AutoresizeTextarea doesn't support the rows prop, we can use it instead of adding new minRows. Like this https://monosnap.com/file/6pOClH0divJQqja7DwO1DJVf6JSO6Z. What do you think?

@maxceem
Copy link
Collaborator

maxceem commented Jul 8, 2019

  1. Regarding 1. as we are ok with the current solution as per [$150] Make text fields expandable in Scope form #3128 (comment) I guess the extra code may be removed from the PR.

@mfikria
Copy link
Contributor Author

mfikria commented Jul 8, 2019

@maxceem seems good to me. I've updated the PRs. Thanks.

Copy link
Collaborator

@maxceem maxceem left a comment

Choose a reason for hiding this comment

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

@mfikria thanks all the code is well updated.

I've just found one issue during the final testing: if I press Enter 3 times fast, the text got truncated again 😟, see demo video https://monosnap.com/file/6qHQWeRM1kEF1z7DeYHcjXZQTTvfwE

Btw, I've noticed there is a newer version of the package we are using in the NPM not sure if it would help with any of the issues we are facing https://www.npmjs.com/package/react-textarea-autosize.

@mfikria
Copy link
Contributor Author

mfikria commented Jul 9, 2019 via email

@mfikria
Copy link
Contributor Author

mfikria commented Jul 9, 2019

@maxceem i've updated the library. Hope this can resolve the issue

@maxceem
Copy link
Collaborator

maxceem commented Jul 9, 2019

@mfikria, unfortunately, it didn't help. Especially it can be easily reproduced in FireFox, see demo video https://monosnap.com/file/iOeebmreBl870v73wVPYAKPZVPMKQq

The text is truncated when I quickly add lines using Enter or remove lines using Backspace, can you reproduce it?

@mfikria
Copy link
Contributor Author

mfikria commented Jul 9, 2019

@maxceem ok noted will try it in firefox

@mfikria
Copy link
Contributor Author

mfikria commented Jul 10, 2019

@maxceem how about change the library to https://www.npmjs.com/package/react-autosize-textarea ?

@maxceem
Copy link
Collaborator

maxceem commented Jul 10, 2019

@mfikria let's give it a try. Could you please, create a separate PR with that library, just in case if the current library works better, so we can keep it.

@maxceem
Copy link
Collaborator

maxceem commented Jul 11, 2019

Hey, @mfikria any luck with another library?

@mfikria
Copy link
Contributor Author

mfikria commented Jul 11, 2019

@maxceem no, https://www.npmjs.com/package/react-autosize-textarea and current library use the same core library which is https://www.npmjs.com/package/autosize. The alternative is rewriting the library from scratch but it need too much effort

@maxceem
Copy link
Collaborator

maxceem commented Jul 11, 2019

Got it @mfikria. Thank you for looking into it, I think we would be fine with the way it works currently as it looks like further fixes would take too much efforts.

Copy link
Collaborator

@maxceem maxceem left a comment

Choose a reason for hiding this comment

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

Works good in most cases.

There is still some edge-case issue:
it can be easily reproduced in FireFox, see demo video https://monosnap.com/file/iOeebmreBl870v73wVPYAKPZVPMKQq
The text is truncated when I quickly add lines using Enter or remove lines using Backspace, can you reproduce it?

Though it looks like efforts to fix this case would be too big. So we use the current fix provided in the react-components repo topcoder-platform/react-components#321 and would log a separate issue for the left edge-case.

@maxceem maxceem merged commit f8b3240 into topcoder-archive:cf18 Jul 11, 2019
@vikasrohit
Copy link

@maxceem could you please log this summary in the issue as well so that we can close that issue with a final note.

@maxceem
Copy link
Collaborator

maxceem commented Jul 15, 2019

@vikasrohit summaries on the issue page and created a follow-up issue for the edge-case.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants