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 basic cookie store #969

Closed
wants to merge 12 commits into from
Closed

Conversation

Tanja-4732
Copy link

@Tanja-4732 Tanja-4732 commented Nov 15, 2023

Description

Contribution Checklist:

  • The pull request only addresses one issue or adds one feature.
  • The pull request does not introduce any breaking changes
  • I have added screenshots or gifs to help explain the change if applicable.
  • I have read the contribution guidelines.
  • Create an issue and link to the pull request. Auth: Cookies #968

Note: Keeping the PR small and focused helps make it easier to review and merge. If you have multiple changes you want to make, please consider submitting them as separate pull requests.

Publishing to New Package Managers

Please see here for more information.

My checklist

  • Figure out how this project is structured
  • Find a place/concept on where to put which new buttons/widgets
  • Implement the cookie store (needs feedback for how to handle persistence)

closes #968

@Tanja-4732
Copy link
Author

Some progress made already:

see #968 (comment)

@Tanja-4732
Copy link
Author

Tanja-4732 commented Nov 18, 2023

gotta love some good rebase ✨

literally best git command

@Tanja-4732 Tanja-4732 changed the title [WIP] Add a basic cookie store Add a basic cookie store Nov 19, 2023
@Tanja-4732 Tanja-4732 marked this pull request as ready for review November 19, 2023 11:32
@Tanja-4732
Copy link
Author

@helloanoop pls review; I hope the code works;

Tell me how to refactor & add screenshots/docu to meet the projects goals; thanks

@helloanoop
Copy link
Contributor

Hi @Tanja-4732

Appreciate all the work, but won't be able to get this in. Below are the issues

  1. The PR considers cookies to be a collection level construct, where as we need it to be at the app level
  2. The cookie implementation does not take into consideration the logic of mapping the cookies to domains. Also the expiry time of cookies

Since this a key feature and many folks are blocked, I went ahead and have implemented the cookie support e1a96e0

However, there is still a lot more work needed on cookie support, specially around adding, editing and deleting cookies.

@Tanja-4732
Copy link
Author

@helloanoop

Thanks for your efforts in

  1. Reviewing my PR
  2. Writing a "proper" implementation yourself

Do you think these additional changes should be made in a new PR?

@helloanoop
Copy link
Contributor

@Tanja-4732 Yes, there is work left on adding, editing and deleting cookies.

Let's pick one at a time.

When you find some time, it'd be great of you could work on support for deleting cookies.
This would be basically a delete icon in each cookie row. Clicking on delete would delete the cookie in the cookie jar.

@helloanoop
Copy link
Contributor

@Tanja-4732 Closing this PR.

In case you are working on support for deleting cookies, please sync your fork's main with usebruno:main and cut a new branch feature/delete-cookies for the delete cookie work

@helloanoop helloanoop closed this Nov 21, 2023
@Tanja-4732
Copy link
Author

@Tanja-4732 Closing this PR.

In case you are working on support for deleting cookies, please sync your fork's main with usebruno:main and cut a new branch feature/delete-cookies for the delete cookie work

Will do soon enough 👍 ✨

@Tanja-4732
Copy link
Author

@Tanja-4732 Closing this PR.
In case you are working on support for deleting cookies, please sync your fork's main with usebruno:main and cut a new branch feature/delete-cookies for the delete cookie work

Will do soon enough 👍 ✨

@helloanoop is this still relevant?

I could port my existing UI-work to your cookie implementation.

Should I?
I'm just asking if that's already in the works or not

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.

Auth: Cookies
2 participants