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

UI Filters (new branch) #1735

Merged
merged 94 commits into from
Apr 18, 2019
Merged

Conversation

tfamula
Copy link
Contributor

@tfamula tfamula commented Apr 5, 2019

This is the third PR for the Filters, which uses a new branch with a current trunk state and also solves all the conflicts from #1253.
#1253 is closed now and the final discussion/review will be continued here.

@klees
Copy link
Member

klees commented Apr 16, 2019

Hi @tfamula,

do we still need the two older PRs (#1193, #1253) to be open? Will you ping me and @Amstutz if this is ready for review?

Best regards!

This was referenced Apr 16, 2019
@tfamula
Copy link
Contributor Author

tfamula commented Apr 16, 2019

Hi @Amstutz and @klees!
I tackled the points from our VC on 14 march 2019 (#1253 (comment)):

  • Expanding and collapsing the filter is possible in all states of the filters now, activated and deactivated, on desktop and mobile. There is no longer full round-trip by doing this, because Ajax is used now.

  • The task with the getUpdateOnLoadCode method is still open. I would really like to put this to the Roadmap for now. I would be very happy if we could merge the Filters like they are currently. After that, I will talk with @alex40724 and @Amstutz if this open task will be done by me, too.
    Shall I write the entry for the Roadmap? Because in the minutes from 14 march we wrote that this will be done by by @klees if needed.

  • Furthermore, I had to spent some time with the unit tests because of switching to a current trunk state, the PHPUnit8 migration and the changes in the Filter itself of course.
    I also figured out that some of the tests cannot be executed correctly due to ilTemplate. @xus told me that there are some problems with it in the trunk at the moment. This goes also for existing tests, you can e.g. have a look at the Forms. Nevertheless, I wanted to be finished with everything and decided to make a little detour to make the Filter tests ready. At 99%, the tests should pass when the ilTemplate-problem is solved ;-)

That's all from me. I hope you are happy with the Filters like they are now and look forward to a merge in the next days :-)

Many greetings

@Amstutz
Copy link
Contributor

Amstutz commented Apr 17, 2019

Hi @tfamula

IMO, the points discussed and listed in #1253 have been taken care of. I also advocate to merge this soon. If more issues occur, we can handled them as bugs. I am also fine with handlinge getUpdateOnLoadCode as entry in the Roadmap. @klees as soon as you are fine with the PR, press the merge button to avoid upcoming conflicts.

Greetings,
Timon

@klees
Copy link
Member

klees commented Apr 18, 2019

Hi @tfamula,

I opened a PR for the roadmap-entry against your initial repo. I'm still not happy with this, since Input now advertises an API that it does not deliver. This will need to be fixed soon and I hope that you will be available for this. Please merge that PR and update the two conflicts, then I'll press the button.

Best regards!

@klees klees merged commit 8668800 into ILIAS-eLearning:trunk Apr 18, 2019
@tfamula tfamula deleted the 5_4_ks_filter3 branch February 23, 2023 11:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants