-
Notifications
You must be signed in to change notification settings - Fork 15
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
Extend edit series block #1058
Extend edit series block #1058
Conversation
Thanks for the PR! I don't have time to look at it in detail yet, but this feature seems reasonable! And you can ignore the failing "admin UI check" for now, that's likely a temporary failure and not caused by your PR. Will come back to this PR soon (TM). |
This pull request has conflicts ☹ |
FYI: we will merge another PR that will cause some conflict with this one soon, so probably not worth it starting to resolve conflicts now. We did that because we wanted to include a bunch of stuff in a small release very soon, while yours would be in the release afterwards. If you want, we can resolve the conflicts for you, just let me know. |
a02515c
to
9ab900b
Compare
I changed the CSS of the edit series form because the 400px max width wasn't enough to display all options. This is maybe a controversial change (9ab900b) |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, your PR is next, I promise 🙈 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks really good! I only have very few small comments. After those are fixed and conflicts are resolved, this can be merged.
The offer still stands that I can rebase/fix conflicts for you. Then I can also fix my comments. Just let me know.
EDIT: and for the record, the failing "admin ui" GH action is no problem. Changing NewSeries
would potentially break the admin UI, but you are only changing NewSeriesBlock
. Due to an incorrect startsWith
in our test script, it fails. So yeah: nothing to worry about. Lets just ignore it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I completely forgot to actually test it. It's clearly time to stop working for today.
I found one additional thing, but other than that it works nicely!
frontend/src/routes/manage/Realm/Content/Edit/EditMode/Series.tsx
Outdated
Show resolved
Hide resolved
895fea3
to
76402c3
Compare
Adds new series block configurable default values. - Extend video list order with the `A-Z` and `Z-A` options. - Add video list view options (List, Gallery, Slider) Signed-off-by: Gregor Eichelberger <gregor.eichelberger@tuwien.ac.at>
Signed-off-by: Gregor Eichelberger <gregor.eichelberger@tuwien.ac.at>
Fixing the migration script for existing databases by splitting the `alter table` statement into two parts. The first statement adds the new `videolist_view` column and sets the default value for existing rows. The second statement removes the column default and updates the constraints. Signed-off-by: Gregor Eichelberger <gregor.eichelberger@tuwien.ac.at>
76402c3
to
4eda5b1
Compare
780dddc
to
8052737
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks again!
Adds new series block configurable default values.
A-Z
andZ-A
options.