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

Make slugfield scrollable #1991

Merged
merged 1 commit into from
Jan 25, 2023
Merged

Make slugfield scrollable #1991

merged 1 commit into from
Jan 25, 2023

Conversation

PeterNerlich
Copy link
Contributor

@PeterNerlich PeterNerlich commented Jan 10, 2023

Short description

Make slugfield usable, even if URI prefix is very long.

Proposed changes

  • make slugfield scrollable by setting overflow: auto on the container
  • ensure input is never at 0 width
  • reduce label opacity to $\frac{2}{3}$ for better distinction from input

Side effects

  • Other places using .slug-field will be affected. We estimate this to be fine.

Resolved issues

Fixes: #1552 (Slug field layout broken when permalink is too long)


Pull Request Review Guidelines

@PeterNerlich PeterNerlich requested a review from a team as a code owner January 10, 2023 11:02
@codeclimate
Copy link

codeclimate bot commented Jan 10, 2023

Code Climate has analyzed commit e97bc26 and detected 0 issues on this pull request.

The test coverage on the diff in this pull request is 100.0% (50% is the threshold).

This pull request will bring the total coverage in the repository to 74.8% (0.0% change).

View more on Code Climate.

@MizukiTemma
Copy link
Member

Thank you for PR 👍

I guess the issue number mensioned in the PR description is not correct...

Copy link
Member

@timobrembeck timobrembeck left a comment

Choose a reason for hiding this comment

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

Thanks a lot, this fixes the issue! 👍

Could you add a changelog entry for this?

If the slug itself is longer than the input field, we have a "stacked" scrolling now, I was wondering whether we could combine this, so that the input field automatically grows when longer slugs are entered so we only have one layer of scrolling, but I can imagine this is probably a bit complicated, so only improve this if it doesn't cost you too much time 😅
I'm already happy with the current solution! 👍

Screenshot 2023-01-10 at 19-04-46 Integreat Redaktionssystem

I guess the issue number mensioned in the PR description is not correct...

True, I fixed it

@timobrembeck
Copy link
Member

Oh, and just a minor addition to my comment:
Please separate the commit subject from body with a blank line, commit subjects with more than 72 characters are a bit hard to read:
Screenshot 2023-01-10 at 20-22-35 Make slugfield scrollable by PeterNerlich · Pull Request #1991 · digitalfabrik_integreat-cms

So I would use e.g. the following commit message instead:

Make slugfield scrollable

- Make slugfield scrollable by setting `overflow: auto` on the container
- Ensure `input` is never at 0 width
- Reduce label opacity to 60% for better distinction from input

Copy link
Contributor

@charludo charludo left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@PeterNerlich
Copy link
Contributor Author

If the slug itself is longer than the input field, we have a "stacked" scrolling now, I was wondering whether we could combine this, so that the input field automatically grows when longer slugs are entered so we only have one layer of scrolling, but I can imagine this is probably a bit complicated [...]

Yes, that is not possible without using JS/TypeScript magic that might act differently for different browsers, and since it would add probably quite a bunch of complexity and we don't have a very clear structure where this logic resides and how it is tested, if at all (at least that we know of), Ingo strongly favoured not investing more time, and I agree. I would have ideas how to hack it, though.

Copy link
Member

@timobrembeck timobrembeck left a comment

Choose a reason for hiding this comment

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

Ingo strongly favoured not investing more time, and I agree.

Sounds reasonable! Yes, this leaves us with the changelog entry and the commit message.

- Make slugfield scrollable by setting `overflow: auto` on the container
- Ensure `input` is never at 0 width
- Reduce label opacity to 60% for better distinction from input

Co-authored-by: Gaston69 <ingo@von-komorowski.name>
Copy link
Member

@timobrembeck timobrembeck left a comment

Choose a reason for hiding this comment

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

👍

@PeterNerlich PeterNerlich merged commit dbffd36 into develop Jan 25, 2023
@PeterNerlich PeterNerlich deleted the scrollable-slugfield branch January 25, 2023 21:17
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.

Slug field layout broken when permalink is too long
4 participants