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

feat: bookmark organizations #94

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

feat: bookmark organizations #94

wants to merge 4 commits into from

Conversation

singlamohit1
Copy link
Collaborator

@singlamohit1 singlamohit1 commented Feb 5, 2023

Fixes #85

@beingnoble03
Copy link
Collaborator

beingnoble03 commented Feb 6, 2023

@singlamohit1

  • I think it would be better if all the cards in a row have the same height.
  • Can we preserve the app header when we shift to /stars? It would be nice to add search in the bookmark page.
  • I think we should replace stars with bookmarks throughout the app.

@beingnoble03
Copy link
Collaborator

@nishantwrp @singlamohit1 your views?

@beingnoble03
Copy link
Collaborator

@singlamohit1 are you still working on this? Please address my comments. Thanks!

@singlamohit1 singlamohit1 self-assigned this Feb 19, 2023
@singlamohit1 singlamohit1 added the enhancement New feature or request label Feb 19, 2023
Copy link
Collaborator

@beingnoble03 beingnoble03 left a comment

Choose a reason for hiding this comment

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

I think there should an option to bookmark an organization in it's info page too.
Screenshot 2023-02-19 at 6 11 27 PM

For design, I think we can do the same thing, we did in the Organizations List Page.
Your views @nishantwrp @singlamohit1 ?

@beingnoble03
Copy link
Collaborator

After an offline discussion with @nishantwrp, got to know that this was already discussed. Please ignore.

@beingnoble03 beingnoble03 self-requested a review April 9, 2023 05:19

const getIfOrgSaved = orgname => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we change this to getIsOrgBookmarked?

const OrgCard = ({ data }) => {
const [ifOrgNotSaved, setIfOrgSaved] = useState(
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we change this to [isOrgBookmarked, setIsOrgBookmarked]?

@@ -20,6 +36,13 @@ const OrgCard = ({ data }) => {
})
.reverse()

const addOrRemoveBookmark = value => {
let isPresent = getIfOrgSaved(value)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we change this to isBookmarked?

@@ -20,6 +36,13 @@ const OrgCard = ({ data }) => {
})
.reverse()

const addOrRemoveBookmark = value => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we change this to toggleBookmark?

localStorage.setItem("gsoc_orgs", JSON.stringify(curr_orgs))
}
return
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
}
}

return
}

export function removeOrgToShortlist(orgname) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we change this to removeOrgFromBookmarks?

@@ -0,0 +1,72 @@
import { createSlice } from "@reduxjs/toolkit"

export function addOrgToShortlist(orgname) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we change this to addOrgToBookmarks?

Copy link
Collaborator

@beingnoble03 beingnoble03 left a comment

Choose a reason for hiding this comment

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

Some variable name changes. Rest LGTM.

@beingnoble03 beingnoble03 changed the title made feature to save orgs feat: bookmark organizations Apr 9, 2023
@singlamohit1
Copy link
Collaborator Author

@nishantwrp Can you pls review the PR. It has been months now. I want to work on more features once this gets live.

@singlamohit1
Copy link
Collaborator Author

@beingnoble03 Can you review the PR since Nishant seems busy

@beingnoble03
Copy link
Collaborator

@singlamohit1 you didn't accept the requested changes.

Repository owner deleted a comment from singlamohit1 Nov 14, 2023
@beingnoble03
Copy link
Collaborator

@nishantwrp please review.

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

Successfully merging this pull request may close these issues.

Bookmark/shortlist orgs
2 participants