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

Added 'My Project' section in hackathon projects #6494

Merged
merged 2 commits into from
Jun 4, 2020
Merged

Added 'My Project' section in hackathon projects #6494

merged 2 commits into from
Jun 4, 2020

Conversation

imtiaz101325
Copy link
Contributor

@imtiaz101325 imtiaz101325 commented Apr 23, 2020

Description

Added 'My Project' section in hackathon projects page

  • filtered existing data with user id
  • duplicated some markup
Refers/Fixes

Refs: #6442

Testing

If user is logged in and has matching projects the new section My Projects now show those projects while the section All Projects show all projects

image

Tablet
image

Mobile
image

@codecov
Copy link

codecov bot commented Apr 23, 2020

Codecov Report

Merging #6494 into master will increase coverage by 0.02%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #6494      +/-   ##
==========================================
+ Coverage   26.67%   26.69%   +0.02%     
==========================================
  Files         293      293              
  Lines       27878    27878              
  Branches     4113     4113              
==========================================
+ Hits         7436     7442       +6     
+ Misses      20176    20170       -6     
  Partials      266      266              
Impacted Files Coverage Δ
app/dashboard/embed.py 31.60% <0.00%> (+3.44%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 44e41e1...9193241. Read the comment docs.

Copy link
Member

@thelostone-mc thelostone-mc left a comment

Choose a reason for hiding this comment

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

  • Could you add in mobile + tablet preview (just to double check)
  • Left a few minor comments
  • This code is already used in index-vue?
    Let's dry the code. Make it into a component hackathon-projects-grid and throw it into vue-components.js

</div>
</div>
</div>
<h1 v-if="userProjects.length > 0" class="font-title title ml-2 mt-4">All Projects</h1>
Copy link
Member

Choose a reason for hiding this comment

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

^ seems like userProjects.length > 0 is repeated!
Wrap it within a <template> to make the code cleaner

<div class="my-3 mb-2 text-left">
<b class="font-weight-bold font-smaller-3">Team Members</b>
<div class="mt-1">
<a v-for="profile in project.profiles" :href=`/profile/${profile.handle}` class="">
Copy link
Member

Choose a reason for hiding this comment

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

remove empty class

@thelostone-mc thelostone-mc marked this pull request as draft April 28, 2020 10:20
@imtiaz101325
Copy link
Contributor Author

imtiaz101325 commented Apr 29, 2020

Hi @thelostone-mc sorry for taking my time to follow up on this.

I have used a template and a for loop to avoid repeating the markup and cleaned up the empty class and style attributes.

@imtiaz101325
Copy link
Contributor Author

Sorry about that. Pressed Close and comment by mistake.

@thelostone-mc thelostone-mc marked this pull request as ready for review May 3, 2020 04:02
@PixelantDesign
Copy link
Contributor

PixelantDesign commented May 13, 2020

@imtiaz101325 would you mind updating the style of buttons to look like this?

View Prize button using an out line and blue text.
Also add text and chat icon to button using an outline style.

Screen Shot 2020-05-13 at 9 22 00 AM

Thanks!

@thelostone-mc
Copy link
Member

@imtiaz101325 the section 1 /2 seems very unclear ->
could we make it into reusable vue component

@ [[ profile.handle ]]
</b-dropdown-item-button>
</b-dropdown>
<template v-for="section in 2">
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we simplify the code here and just repeat the html here rather than iterating with the section var and then checking it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That was my initial thought and I did repeat the markup on the previous commit. I could just revert to that.

@danlipert danlipert marked this pull request as draft May 13, 2020 14:48
@imtiaz101325
Copy link
Contributor Author

imtiaz101325 commented May 13, 2020

@thelostone-mc I wanted to use components but the markup is currently using inline templates and I could not figure out how to use a component here. Which is why tried to use a loop to avoid repetition instead.

@PixelantDesign
Copy link
Contributor

Thanks for the reply @imtiaz101325, what do you suggest @danlipert ?

@PixelantDesign
Copy link
Contributor

@imtiaz101325 could you add an edit button as @octavioamu has suggested? this would only be visible to the person who created the project. Thanks! Happy to tip for this addition.

- filtered existing data with user id
- refactored card markup into component
- added edit button to cards
@imtiaz101325 imtiaz101325 marked this pull request as ready for review May 24, 2020 23:11
@imtiaz101325 imtiaz101325 requested a review from danlipert May 24, 2020 23:12
@imtiaz101325
Copy link
Contributor Author

imtiaz101325 commented May 24, 2020

@PixelantDesign I've added the edit button. Also rebasing the branch with master fixed the button styles. Here's a preview:

image

@imtiaz101325
Copy link
Contributor Author

@thelostone-mc @danlipert refactored the card markup into a component. Please let me know if there's any other changes needed.

@PixelantDesign
Copy link
Contributor

Thank you @imtiaz101325!

@thelostone-mc thelostone-mc merged commit 18962f9 into gitcoinco:master Jun 4, 2020
@PixelantDesign
Copy link
Contributor

Can we deploy?

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.

5 participants