-
-
Notifications
You must be signed in to change notification settings - Fork 813
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
Updated the Upcoming Events list UI on the Organization dashboard. #1366
Updated the Upcoming Events list UI on the Organization dashboard. #1366
Conversation
Our Pull Request Approval ProcessWe have these basic policies to make the approval process smoother for our volunteer team. Testing Your CodePlease make sure your code passes all tests. Our test code coverage system will fail if these conditions occur:
The process helps maintain the overall reliability of the code base and is a prerequisite for getting your PR approved. Assigned reviewers regularly review the PR queue and tend to focus on PRs that are passing. ReviewersWhen your PR has been assigned reviewers contact them to get your code reviewed and approved via:
Reviewing Your CodeYour reviewer(s) will have the following roles:
CONTRIBUTING.mdRead our CONTRIBUTING.md file. Most importantly:
Other
|
@AmitSharma512 you need to import the icon, not the circle or background associated with them, otherwise we cannot use the same icon else where, also don't add "new" to the file name of icons. Use flex inside the time element, so that instead of overflowing it moves to the next line, for Date its okay but if there is time involved too, it may overflow |
… into Dashboard-UI-Design
@@ -2,7 +2,10 @@ | |||
position: relative; | |||
display: flex; | |||
align-items: center; | |||
padding: 0.75rem 0; | |||
padding: 1rem 3rem 1rem 1.5rem; | |||
border: 1px solid #dddddd; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use css variable here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rishav-jha-mech can you give me some idea about how to use the CSS variables ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
like this,
border: 1px solid var(--bs-gray-200);
--bs-gray-200
this variable is from bootstrap
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rishav-jha-mech, can I write inline bootstrap CSS variables in the classname? Is that ok to write ??
font-style: normal; | ||
font-weight: 600; | ||
line-height: 18px; | ||
color: #000000; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use css variable from bootstrap here too
@AmitSharma512 datesnew.svg and eventsnew.svg are the same use any one only and i see no use of appending new to their file name so please remove that, I can see you have not applied inherit to the colour props in them, the svgs should not have any colour by default, so that we can reuse wherever we want in our project. Please follow how we are using svgs in Leftdrawers, you will get the idea |
@rishav-jha-mech datesnew ad eventsnew are different, they are the new logo from the figma file that's why i have appended new to their name. |
@rishav-jha-mech I can only download the default color logo from the figma file, i cannot download the SVG without any color as i have only preview access to the figma file. |
@AmitSharma512 you need to modify the svg file by yourself, check for these attributes in svg file like fill = "#some-colour-hash" OR stroke = "#some-colour-hash" and change it to fill = "current"
or
stroke = "current" Check LeftDrawer component for reference |
… into Dashboard-UI-Design
… into Dashboard-UI-Design
… into Dashboard-UI-Design
… into Dashboard-UI-Design
@rishav-jha-mech, I have made all the required changes; please merge the PR. |
… into Dashboard-UI-Design
@rishav-jha-mech please review it |
src/assets/svgs/locationnew.svg
Outdated
<svg width="16" height="16" viewBox="0 0 16 16" fill="none" xmlns="http://www.w3.org/2000/svg"> | ||
<path fill-rule="evenodd" clip-rule="evenodd" d="M7.99999 14.1257L8.48075 13.584C9.02627 12.9593 9.51694 12.3665 9.95351 11.8027L10.3139 11.3273C11.8187 9.29983 12.5714 7.69069 12.5714 6.50136C12.5714 3.96269 10.5249 1.90479 7.99999 1.90479C5.47503 1.90479 3.42856 3.96269 3.42856 6.50136C3.42856 7.69069 4.18132 9.29983 5.68608 11.3273L6.04646 11.8027C6.66927 12.6007 7.32088 13.3751 7.99999 14.1257Z" stroke="#31BB6B" stroke-linecap="round" stroke-linejoin="round"/> | ||
<path d="M7.99998 8.38093C9.05195 8.38093 9.90474 7.52814 9.90474 6.47617C9.90474 5.4242 9.05195 4.57141 7.99998 4.57141C6.94801 4.57141 6.09521 5.4242 6.09521 6.47617C6.09521 7.52814 6.94801 8.38093 7.99998 8.38093Z" stroke="#31BB6B" stroke-linecap="round" stroke-linejoin="round"/> | ||
</svg> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why there is hardcoded value for stroke, add current there also
@AmitSharma512 fix the failing tests |
@rishav-jha-mech made the required changes; please review it. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #1366 +/- ##
========================================
Coverage 97.34% 97.34%
========================================
Files 136 136
Lines 3572 3578 +6
Branches 1039 1045 +6
========================================
+ Hits 3477 3483 +6
Misses 90 90
Partials 5 5 ☔ View full report in Codecov by Sentry. |
… into Dashboard-UI-Design
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@AmitSharma512 please rename the svg files with something relevant to their use. Eg. cardItemDate
, cardItemEvent
and cardItemLocation
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rest LGTM.
@rishav-jha-mech Please review. You had some requested changes that need to be verified |
… into Dashboard-UI-Design
Please fix the introspection test error |
@palisadoes, it is not because of the files changed by me; the error is in the mutations file, and I haven't changed the mutation files. I am getting this error because of the previous commit I pulled from the branch. |
@palisadoes The error is because of the files changed in the PR #1384, The above screenshot shows that the error is in the PR #1384, i guess the PR has to be reverted. |
All the suggestions from @rishav-jha-mech have been made, you can verify them. |
@beingnoble03 can you please review the changes requested by @rishav-jha-mech , so that I can work on new issues :))) |
… into Dashboard-UI-Design
f5fa13d
into
PalisadoesFoundation:develop
What kind of change does this PR introduce?
Updated the Upcoming Events list UI on the Organization dashboard.
Issue Number:
Fixes #1358
Snapshots/Videos:
Does this PR introduce a breaking change?
No
Have you read the contributing guide?
Yes