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

relations form vertical scroll #1351

Merged
merged 2 commits into from
Nov 8, 2024
Merged

Conversation

gythaogg
Copy link
Contributor

@gythaogg gythaogg commented Nov 7, 2024

This pull request includes updates to the CSS for the relation dialog and the addition of a new form in the sample_project application.

CSS updates:

Form additions:

  • sample_project/forms.py: Added a new form class LivesInForm that inherits from RelationForm and includes a notes field with a large textarea widget.

@gythaogg gythaogg requested a review from b1rger November 7, 2024 11:38
@gythaogg gythaogg force-pushed the gythaogg/relations-form-vert-scroll branch from e6d7222 to 511daec Compare November 7, 2024 12:01
Copy link
Contributor

@b1rger b1rger left a comment

Choose a reason for hiding this comment

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

Thanks! That bug already annoyed me and we also have a bug report in oebl for that acdh-oeaw/apis-instance-oebl-pfp#354 !

I'm not sure if it is worth it to add the additional fields to the sample project, but that is definitely up for discussion. If we try to showcase every css fix in the sample project it will soon get overly complex and IIRC the idea of the sample_project was also for external users to have an easily understandable test project they can use to get to know APIS.
Especially in this case, the suggested change did not lead to the wanted showcasing of the bug, because my screen is big enough so that the added form fields also visible without the css fix.

An alternatative to the change of the models (which also bugs me a bit, because it changes the datamodel and adds migrations) would be to override the default form for a relation:

from django import forms
from apis_core.relations.forms import RelationForm


class LivesInForm(RelationForm):
    notes = forms.CharField(widget = forms.Textarea(attrs={"rows": "70"}), required=False)

Also: could you make the commit message a bit more verbose (I suppose the PR message was generated, but some of that information could be part of the commit message, i.e. "handle overflow separately for the x and y axes, making the y-axis scrollable").
"vertical scroll in relations dlg" is a bit scarce, maybe "handle overflow in relation dialog" and then in the message describe the problem and "handle overflow separately for the x and y axes, making the y-axis scrollable" from the PR description.

In general I think the commit message should be the primary source for information about the changes, not the PR description.

@gythaogg gythaogg force-pushed the gythaogg/relations-form-vert-scroll branch 2 times, most recently from 320b934 to 3e9e5dc Compare November 8, 2024 08:21
@gythaogg gythaogg marked this pull request as draft November 8, 2024 08:21
@gythaogg gythaogg marked this pull request as ready for review November 8, 2024 08:22
@gythaogg
Copy link
Contributor Author

gythaogg commented Nov 8, 2024

In general I think the commit message should be the primary source for information about the changes, not the PR description.

Yeah, that's fair. I never go and look at a PR description when I time travel to find when/why something changed .

When the form to add new relations runs longer the dialog height,
all its fields and the submit button must still be accessible.
This change handles the overflow separately for the x an y axis,
making the y-axis scrollable.
@b1rger b1rger force-pushed the gythaogg/relations-form-vert-scroll branch from 3e9e5dc to c0f4832 Compare November 8, 2024 09:24
Copy link
Contributor

@b1rger b1rger left a comment

Choose a reason for hiding this comment

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

(added a comment to the form and wrote out "dialog" in the commit message)

@gythaogg
Copy link
Contributor Author

gythaogg commented Nov 8, 2024

(added a comment to the form and wrote out "dialog" in the commit message)

Damn - I had to shorten it because my editor said it was too long. I need to align the commit message linter config with my own

@gythaogg gythaogg merged commit 4b08b6f into main Nov 8, 2024
13 checks passed
@gythaogg gythaogg deleted the gythaogg/relations-form-vert-scroll branch November 8, 2024 09:44
gythaogg added a commit that referenced this pull request Nov 12, 2024
reverted modal dialog overflow to `visible` in the new relations form so
that when the dialog height is shorter than the dropdown, the dropdown
remains accessible without having to scroll the dialog.

introduced max-height and auto scroll option for
`#relation-dialog-content` so that when the contents of the dialog
overflows the dialog height, the dialog is scrollable (maintains the
behaviour of fix #1351)

closes #1359
gythaogg added a commit that referenced this pull request Nov 12, 2024
reverted modal dialog overflow to `visible` in the new relations form so
that when the dialog height is shorter than the dropdown, the dropdown
remains accessible without having to scroll the dialog.

introduced max-height and auto scroll option for
`#relation-dialog-content` so that when the contents of the dialog
overflows the dialog height, the dialog is scrollable (maintains the
behaviour of fix #1351)

closes #1359
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.

2 participants