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

Add a favorites menu in the back end #5592

Merged
merged 16 commits into from Dec 23, 2022
Merged

Conversation

leofeyer
Copy link
Member

@leofeyer leofeyer commented Dec 19, 2022

Implements #5401

TODO

  • Unit tests
  • Check permissions

@leofeyer leofeyer added this to the 5.1 milestone Dec 19, 2022
@leofeyer leofeyer self-assigned this Dec 19, 2022
@leofeyer leofeyer marked this pull request as ready for review December 20, 2022 15:20
@leofeyer leofeyer requested a review from a team December 20, 2022 15:20
Copy link
Member

@Toflar Toflar left a comment

Choose a reason for hiding this comment

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

Wohey, apart from the code reviews, I've checked out the branch and tested it. I've noticed that when I am on a page that is already a favorite, I can still add it as a favorite again - that doesn't make sense, don't you think?

Looks pretty good already, though! 🥳

@leofeyer
Copy link
Member Author

I've noticed that when I am on a page that is already a favorite, I can still add it as a favorite again - that doesn't make sense, don't you think?

Changed in b3cc8a9.

@leofeyer
Copy link
Member Author

This one is ready for review again. 😉

Toflar
Toflar previously approved these changes Dec 21, 2022
Copy link
Member

@Toflar Toflar left a comment

Choose a reason for hiding this comment

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

Look how beautiful this code has turned out with the new permission voter ❤️

I think I can approve this. I have just two UX remarks which I noticed using the feature that are debatable and you might want to look into or also not:

  1. If somehow possible it would be great if the favorite had a default title when I add a new one.
  2. Not sure if it's good to remove the icon from the navigation when already added. It makes the navigation change all the time when I navigate. Maybe disabling it with a different icon indicating that you cannot click would be better.

Again not sure for both. Both can also be changed or added later on. I just wanted to leave the remarks here.

@leofeyer
Copy link
Member Author

I have implemented no. 2 in 7a2f03d.

Now if a URL is a favorite already, the symbol changes and clicking it takes you to the favorites module:

No. 1 is unfortunately much harder to implement. The best predefined value would be the page title, however, since we do not have a request context in the back end, I cannot retrieve it in the listener. We may have to open a separate issue and discuss this in the next call.

@leofeyer leofeyer requested a review from Toflar December 21, 2022 15:02
Copy link
Member

@Toflar Toflar left a comment

Choose a reason for hiding this comment

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

The navigation feels way better (to me) now! Thanks!

Regarding the tilte: Sure, that's something we can still improve.

@leofeyer leofeyer merged commit 6760014 into contao:5.x Dec 23, 2022
@leofeyer leofeyer deleted the feature/favorites branch December 23, 2022 08:04
@contaoacademy
Copy link

Awesome, thanks for the implementation

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants