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 search (filter) for an specific Integration on Homepage's Integrations section #1837

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

Conversation

shubhusion
Copy link

Description

This PR fixes #1798

Notes for Reviewers

Signed commits

  • Yes, I signed my commits.

Signed-off-by: shubhusion <[email protected]>
Copy link

netlify bot commented Jul 31, 2024

Deploy Preview for mesheryio-preview ready!

Name Link
🔨 Latest commit 1011f08
🔍 Latest deploy log https://app.netlify.com/sites/mesheryio-preview/deploys/66b9ad22d863b70008cc50cc
😎 Deploy Preview https://deploy-preview-1837--mesheryio-preview.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@shubhusion
Copy link
Author

@vishalvivekm can you please review this PR ?

@Ashparshp
Copy link

@shubhusion Thanks for your contribution, let's discuss this on the website's call. Please add this as an agenda item to the meeting minutes. The call is scheduled for Monday at 5:30 PM IST.

@shubhusion
Copy link
Author

@Ashparshp, this PR has already been discussed earlier. In the previous PR, I made a mistake while rebasing the branch, which led to merging other commits into that PR. Therefore, I closed the previous PR and created a new one with the necessary specified changes.

@Ashparshp
Copy link

@shubhusion Thanks for the words!

@shubhusion
Copy link
Author

shubhusion commented Aug 5, 2024

@Ashparshp can you review this PR ? Earlier @hargunkaur286 had reviewed it.

Signed-off-by: shubhusion <[email protected]>
@shubhusion
Copy link
Author

@vishalvivekm required changes has been made.

@amitamrutiya
Copy link
Contributor

I found several issues while reviewing your PR. I'm not sure if these are only happening on my end.

Screencast.from.2024-08-12.09-50-39.webm

I am not able to see the cursor icon on the button and can only click certain areas on the buttons below. Other issues are mentioned in the video. Also, the gap between buttons is different from our previous one.

I am using Firefox for testing.

@shubhusion
Copy link
Author

I found several issues while reviewing your PR. I'm not sure if these are only happening on my end.

Screencast.from.2024-08-12.09-50-39.webm

I am not able to see the cursor icon on the button and can only click certain areas on the buttons below. Other issues are mentioned in the video. Also, the gap between buttons is different from our previous one.

I am using Firefox for testing.

@amitamrutiya i have made no modifications in buttons. My task was to add a search filter above the icons.

@amitamrutiya
Copy link
Contributor

I think this PR makes the above mentioned changes.

@shubhusion
Copy link
Author

shubhusion commented Aug 12, 2024

I think this PR makes the above mentioned changes.

@amitamrutiya buttons are working for me.
spacing and other issues i will rectify

@shubhusion
Copy link
Author

shubhusion commented Aug 12, 2024

@amitamrutiya all issues you raised have been rectified. Thanks for the review

@shubhusion
Copy link
Author

@vishalvivekm @sudhanshutech please review this pr

Copy link

@Ashparshp Ashparshp left a comment

Choose a reason for hiding this comment

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

We can fix those issues as well, as @amitamrutiya pointed out. However, let's address only the ones that have been modified for now. We can proceed with the PR if @shubhusion is not going to work on the fix. @amitamrutiya, you can take up the rest in a separate issue if you'd like.

@shubhusion thoughts?

@shubhusion
Copy link
Author

LGTM! We can fix those issues as well, as @amitamrutiya pointed out. However, let's address only the ones that have been modified for now. We can proceed with the PR if @shubhusion is not going to work on the fix. @amitamrutiya, you can take up the rest in a separate issue if you'd like.

@shubhusion thoughts?

@Ashparshp The issues pointed out by @amitamrutiya have been rectified in the last two commits.

@Ashparshp
Copy link

Thank you @shubhusion.

@shubhusion
Copy link
Author

@hargunkaur286 can you please review this pr ?

@@ -1,83 +1,166 @@
<style>
Copy link
Member

Choose a reason for hiding this comment

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

@shubhusion why the existing search bar that is in catalog page is being not reused? Why we have to create new component and styles? Please reuse that

Copy link
Member

Choose a reason for hiding this comment

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

@shubhusion i didn't heard from you...

Copy link
Author

@shubhusion shubhusion Aug 28, 2024

Choose a reason for hiding this comment

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

@sudhanshutech I will make the necessary changes, but I'm a bit busy this week. Please give me some time.

Copy link
Author

Choose a reason for hiding this comment

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

@shubhusion

@shubhusion why the existing search bar that is in catalog page is being not reused? Why we have to create new component and styles? Please reuse that

The styles from the existing search bar on the catalog page were duplicated and adapted for the new search bar on the home page. Some modifications were made to ensure it fits well on the home page.

To reuse the same styles, I will need to create a shared .css file that is accessible to both pages.

@Ashparshp
Copy link

@shubhusion, I'm adding this as an agenda item to the meeting minutes. Please join the evening call and let's take it forward.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add search (filter) for an specific Integration on Homepage's Integrations section
5 participants