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

Fix question header title for extra long text #1393

Merged
merged 1 commit into from Dec 23, 2022
Merged

Conversation

susnux
Copy link
Collaborator

@susnux susnux commented Oct 25, 2022


Currently the title is set to a height of 44px (used 14px padding top, 16px text height, 14px padding bottom) to fit the menu button on the right.
This allows the header to have multiple lines of text by setting only a minimum height of 30px + a fixed padding of 14px on the bottom.

@susnux susnux added bug Something isn't working design Related to the design 3. to review Waiting for reviews labels Oct 25, 2022
@jotoeri
Copy link
Member

jotoeri commented Oct 25, 2022

Uhhm, tbh. I also just had a quick look, i think it should suffice to remove the fixed height? The auto-margin does the rest...
The padding currently moves the text upwards for me, so edit/non-edit are not aligned anymore.

@Chartman123
Copy link
Collaborator

I also had a look at the other input fields and think that we have the same problem there (at least for the options for radio/checkbox)

grafik

@susnux
Copy link
Collaborator Author

susnux commented Oct 25, 2022

Uhhm, tbh. I also just had a quick look, i think it should suffice to remove the fixed height? The auto-margin does the rest... The padding currently moves the text upwards for me, so edit/non-edit are not aligned anymore.

Without padding the description gets quite close to the heading, so currently there is a padding of 14px (44px - 16px -14px) between title and description

@jotoeri
Copy link
Member

jotoeri commented Oct 25, 2022

Can't test it anymore currently, but did the description move at all, if you just deactivate the fixed height (on a one-line title)? Cause the title-line itself keeps the 44px due to the actions iirc. Maybe the margin overlaps, but that I can't tell without testing.
Anyways, if you think the description is too close, better increase the margin of the description. But the text shouldn't move when switching between edit/non-edit mode of the question. (You can test that best, when having a description of three/four lines set.)

Edit:
Or did you test on submit, while I did on create? Then I can imagine your point. 🙈
Just making the height a min-height would probably also be a solution?

@susnux
Copy link
Collaborator Author

susnux commented Oct 25, 2022

I also had a look at the other input fields and think that we have the same problem there (at least for the options for radio/checkbox)

Thats from @nextcloud/vue enforcing a height of 44px instead of min-height for the label.

Just making the height a min-height would probably also be a solution?

Yes, but then there would be 10px padding between title and description for titles with one line and no padding for titles with multiple lines.


I pushed a slightly different version fixing both issues (jumping text and option title), see this screen recording of the changes:

voko.mp4

@susnux susnux requested a review from jotoeri October 27, 2022 01:39
@Chartman123
Copy link
Collaborator

@susnux @jotoeri Should we change the inputs for the title and the options to textareas to reduce the moving of the texts even more?

I'm not that satisfied with the vertically centered alignment of the checkboxes/radios either... Can this be aligned next to the first line of the options?

@jotoeri
Copy link
Member

jotoeri commented Dec 6, 2022

So - sorry. Now also found some time to test this.

Yes, but then there would be 10px padding between title and description for titles with one line and no padding for titles with multiple lines.

You're right, i'm fine with that. Can you add it to the checkbox-label, too? Same thing is currently for long-text answers. 🙈

But then, wanna create a PR on the vue-lib for the multi-line label fixes? Better to fix it there, instead of the deep-change. Or should i go for it?

@hubertbanas

This comment was marked as spam.

Currently the title is set to a fixed height of 44px (used 16px text height + padding around)
to fit the menu button on the right. This change allows the header to have multiple lines of text
by setting only a minimum height + a fixed padding.

And also allows Question options to be longer than one line.

Signed-off-by: Ferdinand Thiessen <rpm@fthiessen.de>
@jotoeri
Copy link
Member

jotoeri commented Dec 23, 2022

But then, wanna create a PR on the vue-lib for the multi-line label fixes?

Done & merged.

I'm not that satisfied with the vertically centered alignment of the checkboxes/radios either.

@Chartman123 generally i'm with you - might just become difficult with the pill-shaped background. 😞

Should we change the inputs for the title and the options to textareas to reduce the moving of the texts even more?

Yeah, why not - Maybe also the NCRichContent as in the MD-PR. #1394 But then as a separate PR? Let's go on here and release with this fix for now?

grafik

@jotoeri jotoeri merged commit 8741fc6 into master Dec 23, 2022
@jotoeri jotoeri deleted the fix/question-heading branch December 23, 2022 16:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review Waiting for reviews bug Something isn't working design Related to the design
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Question not shown correctly with long titles
4 participants