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

feat: Improve CreateComment and CreateReply components (ONEARMY#3656)… #4020

Conversation

johannes-ross
Copy link
Contributor

… Create CSS & JS logic for size change

PR Checklist

PR Type

What kind of change does this PR introduce?

  • Feature (adds functionality)

What is the current behavior?

Site just breaks or doesn't look pretty when you try to comment on a mobile device.
When you type, the textarea never changes size. You can change it's size manually.

What is the new behavior?

Site now looks pretty when you try to comment on a mobile device or a different screensize.
When you type, the textarea now dynamicly changes it's size.

Does this PR introduce a breaking change?

  • No

Git Issues

Closes #3656

@johannes-ross johannes-ross requested a review from a team as a code owner November 26, 2024 18:06
@johannes-ross
Copy link
Contributor Author

I would like to add, that I haven't added new Unit Tests, since this is also just a visual change.

@mariojsnunes
Copy link
Contributor

Thanks @johannes-ross!
My first impression is that it would be a bit hard to maintain. Could it be done with css only?

@johannes-ross
Copy link
Contributor Author

johannes-ross commented Nov 26, 2024

I agree and also think a bit hard is understated. The values I used for the breakpoints are all coming from testing. I don't even understand why they work. Definition of magic numbers.

I've tried to use a CSS only approach. I used the following sources and variations and combinations of it:

The Problem that I always encountered was flickering when getting near the line end. Here is what I've found:

  • The hoped for behaviour is, when you type the first line, we want the character count to be inside the element, but when you reach it, it should snap to the bottom and let you finish that first line. All other lines should be within the new, bigger Line. That caused flickering when using CSS, which I've tried to get rid of, but I never managed to find a solution, except ditching all CSS.
  • When you press enter there should be a new line. Not a problem, can easily be done with CSS. However, it is a problem when trying to combine CSS and JS, because JS needs the values from CSS to calculate the new values. (It's a bit more complicated then that)
  • When you use CSS you cannot widen the input dynamically, because you are trying to use the word-break as an indicator, that you need to add a new line, which isn't a problem except for that very first line.
  • Since we always want to have the same look, another problem was detecting, when we only have one line left, when previously having multiple, since having multiple lines has a bigger bottom padding then the single line. So You need to know, when you reach a character width, that will cause the character counter to go down and increase the bottom padding and know when you've crossed that line again, so you can reduce the bottom padding and snap back into the single line.
  • You also can't just set a character count limit and then break because 5 Ws are way bigger than five is:
    "WWWWW"
    "iiiii"

So using pure CSS doesn't work, because we have to change the width of the text area when we reach the first character count.
Using a combination of CSS and JavaScript doesn't work, because it's just to complex.
Which only leaves the approach of using JS to fix it.

A simple Idea to get rid of all the complex JS and CSS combination would be to just get rid of this:
image

and always have the character count on the bottom line, like in the second one.

I've just noticed, I missed that when the Input is empty, the count shouldn't be displayed. I have to fix that, but I'm going to wait until we've discussed this and found a useful solution.

Our greatest hope is, that the future will fix it:

@davehakkens davehakkens added the Review allow-preview ✅ Has received manual check for malicious code and can be safely built for preview label Nov 26, 2024
@dalibormrska
Copy link
Member

Wow, such a cool contribution @johannes-ross ! Thank you for all the effort, I see now the problem of complexity of that trick and maintenance which I didn't foresee from the perspective of a designer.

I agree that we can simplify a bit. The point with the moving character count was so that the component has only the necessary height in the beginning when it's not active, in case that the user sees several of them at once, so that they don't occupy a big portion of the screen.

This point could be achieved also simply like this:
image

The char count is not shown when the textarea is not active, but once the user clicks into the text area (or types the first character, or whatever makes sense to code) the char count shows below.

What do you think? Would that help?
Cheers!

@dalibormrska
Copy link
Member

Oh and one more thing... Would it be possible to set the font-size of the text-area to be equal to the font-size of the comment? Side note: In the near future it might happen that for mobile, the comments' font size might be lower (14px) than on desktop. But there is no decision yet, so I don't know if it's good to hard-code it.

The current difference between font-sizes:
image

Copy link
Member

@benfurber benfurber left a comment

Choose a reason for hiding this comment

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

Thanks so much @johannes-ross for such a thorough dive into this!

So yes, definitely agree with @mariojsnunes about the maintenance issue.

How's the below for a tighter technical scope?

  1. Go as far as can be achieved with css (i.e. if functionality isn't possible without custom js, that it's not possible.)
  2. There's a readability challenge due to how much styling has to be changed between mobile/desktop so I think it's OK to use the responsive display none appoach at a slightly higher level to have seperate components for mobile/desktop like how it's done on src/pages/Maps/Content/MapView/MapWithList.tsx. Then
  3. As it's available on chrome, I'm very happy for field-sizing to be tried (with a code comment).

A more detailed point...

  • I think the use of Grid can be avoided using 2 above. Then for desktop it's just the badge and button using the space they need and the textbox flexing in the middle and for mobile the icon (which should be done using oa-components' Button with the showIconOnly prop

It's funny how much back and forth can happen on this type of relatively small issue so if there's still changes needed after your next draft maybe we can pair to finish it off?

@benfurber
Copy link
Member

@all-contributors add @johannes-ross for code

Copy link
Contributor

@benfurber

I've put up a pull request to add @johannes-ross! 🎉

@johannes-ross
Copy link
Contributor Author

Hey, just an update. I'm currently really busy and won't be abled to work on this till tomorrow.
Thank you for your patience.

@johannes-ross
Copy link
Contributor Author

I've tried to use as much CSS as possible. The only thing I've used JS for is the focus.

I can remove it, because it adds 2 new Renders, but I don't think it's actually that significant.

Copy link

cypress bot commented Nov 29, 2024

onearmy-community-platform    Run #6640

Run Properties:  status check passed Passed #6640  •  git commit c00a058ab5: chore: tidy-up for house style
Project onearmy-community-platform
Branch Review pull/4020
Run status status check passed Passed #6640
Run duration 04m 56s
Commit git commit c00a058ab5: chore: tidy-up for house style
Committer Ben Furber
View all properties for this run ↗︎

Test results
Tests that failed  Failures 0
Tests that were flaky  Flaky 2
Tests that did not run due to a developer annotating a test with .skip  Pending 0
Tests that did not run due to a failure in a mocha hook  Skipped 0
Tests that passed  Passing 71
View all changes introduced in this branch ↗︎

@benfurber benfurber force-pushed the feat/Improve-CreateComment-and-CreateReply-components branch from 67e82a4 to a398a40 Compare December 3, 2024 14:24
@benfurber benfurber force-pushed the feat/Improve-CreateComment-and-CreateReply-components branch from a398a40 to 54ffc3c Compare December 3, 2024 15:11
@benfurber benfurber force-pushed the feat/Improve-CreateComment-and-CreateReply-components branch from 54ffc3c to 85bb60a Compare December 3, 2024 15:13
@benfurber
Copy link
Member

@mariojsnunes Looks like all the preview branches are failing as they need SUPABASE_PROJECT_ID

@benfurber benfurber force-pushed the feat/Improve-CreateComment-and-CreateReply-components branch from d9a8474 to c00a058 Compare December 5, 2024 11:43
@benfurber
Copy link
Member

Thanks @johannes-ross! Rather than ask you to do a bunch of minior clean-up that's more subjective so I thought I'd just do it. Merging in now.

@benfurber benfurber merged commit 33b0492 into ONEARMY:master Dec 5, 2024
18 checks passed
@onearmy-bot
Copy link
Collaborator

🎉 This PR is included in version 2.15.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

@dalibormrska
Copy link
Member

Heyy, sorry for the bump again. I just noticed that the change is implemented across HowTo and Research modules, but not in the Questions module. Could we do a quick fix and implement it there as well?

Thankssss, great job @johannes-ross once again! Works like a charm. :))

@johannes-ross
Copy link
Contributor Author

Hey, I've looked at the Question Code and I can't quickly find a reason, why it would behave the way you described. @dalibormrska I think we should open a new Ticket for further investigation.

@mariojsnunes
Copy link
Contributor

Heyy, sorry for the bump again. I just noticed that the change is implemented across HowTo and Research modules, but not in the Questions module. Could we do a quick fix and implement it there as well?

Thankssss, great job @johannes-ross once again! Works like a charm. :))

I'll take care of that

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
released Review allow-preview ✅ Has received manual check for malicious code and can be safely built for preview
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

[feature request] Improve CreateComment and CreateReply components
6 participants