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

Remove code from mobile view of setting bounty status as open if github issue is open #1164

Merged
merged 7 commits into from
Dec 26, 2023
Merged

Remove code from mobile view of setting bounty status as open if github issue is open #1164

merged 7 commits into from
Dec 26, 2023

Conversation

gouravmpk
Copy link
Contributor

Describe your changes

Edited StatusPill

Issue ticket number and link

#1143

  • [ ✅ ] Bug fix (non-breaking change which fixes an issue)

Checklist before requesting a review

  • [✅ ] I have performed a self-review of my code
  • [✅ ] I have tested on Chrome and Firefox
  • [ ✅ ] I have tested on a mobile device
  • [✅ ] I have provided a screenshot or recording of changes in my PR if there were updates to the frontend

Video

open.bounty.remove.from.mobal.view.mp4

Note:

I'm not entirely certain if this aligns with the request, but in this pull request, I removed the code in StatusPill that typically sets the bounty status based on GitHub issues. This code previously determined whether the bounty status was 'Open' or 'Closed' based on the GitHub issues status. However, I observed that there is no other property or variable where the status of the bounty is being set. Could you please confirm if this is the intended change?
@kevkevinpal @ecurrencyhodler

@kevkevinpal
Copy link
Contributor

looks good just added one comment cause we should rename this file if we arent using the github user assigned stuff anymore

@gouravmpk
Copy link
Contributor Author

@kevkevinpal is this fine

<Pill isOpen={isOpen}>
<div>{isOpen ? 'Open' : 'Closed'}</div>
</Pill>
<Pill isOpen={isOpen}>{/* <div>{isOpen ? 'Open' : 'Closed'}</div> */}</Pill>
Copy link
Contributor

Choose a reason for hiding this comment

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

we can delete this pill since there is no children to the component

@@ -55,7 +55,7 @@ const W = styled.div`
display: flex;
align-items: center;
`;
export default function GithubStatusPill(props: GithubStatusPillProps) {
export default function StatusPill(props: StatusPillProps) {
const { status, assignee, style } = props;
Copy link
Contributor

Choose a reason for hiding this comment

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

We can delete the status prop since that is no longer being used

Copy link
Contributor Author

@gouravmpk gouravmpk Dec 22, 2023

Choose a reason for hiding this comment

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

Done

@ecurrencyhodler
Copy link
Contributor

@gouravmpk Based on your video, it looks like the bounty is missing the status of: open/assigned/closed. Is that right?

This is what your video shows:
Screenshot 2023-12-23 at 7 57 52 AM

This is what it should show and is currently live in production:
Screenshot 2023-12-23 at 7 58 13 AM

We shouldn't be removing the status of the bounty in mobile you. All we're doing is removing the code that auto-sets the status of the bounty as "open" if the github issue is still open.

@gouravmpk
Copy link
Contributor Author

Hey @ecurrencyhodler The status of the bounty is coming from GitHub issue only there is no other code or rather a property which sets bounty status except GitHub status ,this is my observation is that correct @kevkevinpal

@ecurrencyhodler
Copy link
Contributor

ecurrencyhodler commented Dec 23, 2023

Interesting. Okay if that's the case we can merge this and handle the actual status of the bounty in #1161. Jk I'll need to create a separate issue to handle this.

@gouravmpk
Copy link
Contributor Author

Interesting. Okay if that's the case we can merge this and handle the actual status of the bounty in #1161

Yep it would make sense.

@kevkevinpal
Copy link
Contributor

ya @ecurrencyhodler the status on here is for the github issue and the status of the bounty itself should be separate issues, I the scope of this PR is good as we're just removing the github status.

Will review in a bit

Comment on lines 5 to 36
// interface PillProps {
// readonly isOpen: boolean;
// }
// const Pill = styled.div<PillProps>`
// display: flex;
// justify-content: center;
// align-items: center;
// font-size: 12px;
// font-weight: 300;
// background: ${(p: any) => (p.isOpen ? '#49C998' : '#8256D0')};
// border-radius: 30px;
// border: 1px solid transparent;
// text-transform: capitalize;
// padding: 12px 5px;
// // padding:8px;
// font-size: 12px;
// font-weight: 500;
// line-height: 20px;
// white-space: nowrap;
// border-radius: 2em;
// height: 26px;
// color: #fff;
// margin-right: 10px;
// width: 58px;
// height: 22px;
// left: 19px;
// top: 171px;

/* Primary Green */
// /* Primary Green */

border-radius: 2px;
`;
// border-radius: 2px;
// `;
Copy link
Contributor

Choose a reason for hiding this comment

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

you can delete this if it is not being used

@kevkevinpal
Copy link
Contributor

I think it looks good if you can remove the commented code

@gouravmpk
Copy link
Contributor Author

I think it looks good if you can remove the commented code

Okay I am on it .

@gouravmpk
Copy link
Contributor Author

@kevkevinpal I think this is good to go please check

@kevkevinpal
Copy link
Contributor

looks good to me nice work!

@kevkevinpal kevkevinpal merged commit 3f7c62d into stakwork:master Dec 26, 2023
5 checks passed
elraphty pushed a commit that referenced this pull request Jan 26, 2024
…ub issue is open (#1164)

* #1143

* removing the code in StatusPill which sets the bounty to open or closed from github issues.

* prettier

* changed GithubStatusPill to StatusPill

* removed status

* REMOVED commented part
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.

3 participants