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: Add task request UI #402

Closed
wants to merge 21 commits into from
Closed

feat: Add task request UI #402

wants to merge 21 commits into from

Conversation

TanishqSingla
Copy link
Contributor

@TanishqSingla TanishqSingla commented Jul 13, 2023

closes #275

This PR is following stacked PR pattern. The PR above it are

PR for tests

Desktop View

 

Show
Screenshot from 2023-07-20 18-28-18

Mobile View

  • Status Approved
     
    Show
    image
  • Status Waiting
     
    Show
    image

@cloudflare-workers-and-pages
Copy link

cloudflare-workers-and-pages bot commented Jul 14, 2023

Deploying with  Cloudflare Pages  Cloudflare Pages

Latest commit: 8c12b2d
Status: ✅  Deploy successful!
Preview URL: https://320fb365.dashboard-rds.pages.dev
Branch Preview URL: https://feat-task-request.dashboard-rds.pages.dev

View logs

Copy link
Member

@sahsisunny sahsisunny left a comment

Choose a reason for hiding this comment

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

All looks good but some minor changes are required.

  1. Create the Separate constant file for the constant value.
  2. Create variable for the RGB and HEX value of the color in CSS.
  3. Use already written utils function if possible.
  4. Give the proper name to the variable.
  5. Remove the console.log from the code.
  6. Move the utils function to the separate file.

Use variables for colors
Use constants for messages
Extract constants into a separate file
sahsisunny
sahsisunny previously approved these changes Jul 15, 2023
taskRequests/style.css Outdated Show resolved Hide resolved
Copy link

@bharati-21 bharati-21 left a comment

Choose a reason for hiding this comment

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

Please write tests for this change.

@TanishqSingla
Copy link
Contributor Author

Please write tests for this change.

Tests contain a lots of changes which is why they are being held in a separate PR #407

taskRequests/script.js Show resolved Hide resolved
taskRequests/style.css Outdated Show resolved Hide resolved
sahsisunny
sahsisunny previously approved these changes Aug 1, 2023
Pratiyushkumar
Pratiyushkumar previously approved these changes Aug 1, 2023
Copy link
Contributor

@Pratiyushkumar Pratiyushkumar left a comment

Choose a reason for hiding this comment

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

LGTM

Pratiyushkumar
Pratiyushkumar previously approved these changes Aug 6, 2023
Copy link
Contributor

@Pratiyushkumar Pratiyushkumar left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@RitikJaiswal75 RitikJaiswal75 left a comment

Choose a reason for hiding this comment

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

https://jam.dev/c/273db6f0-6a47-4aa0-bb94-8c87aaaefce8

Check this out it always says that i am not authenticated to view this page even though i am super user in staging.

using the above link you can see my network and console logs

@Achintya-Chatterjee
Copy link
Member

https://jam.dev/c/273db6f0-6a47-4aa0-bb94-8c87aaaefce8

Check this out it always says that i am not authenticated to view this page even though i am super user in staging.

Using the above link you can see my network and console logs

@RitikJaiswal75 can you please tell me what causing this error

@Achintya-Chatterjee
Copy link
Member

https://jam.dev/c/273db6f0-6a47-4aa0-bb94-8c87aaaefce8
Check this out it always says that i am not authenticated to view this page even though i am super user in staging.
Using the above link you can see my network and console logs

@RitikJaiswal75 can you please tell me what causing this error

@RitikJaiswal75
Copy link
Contributor

@RitikJaiswal75 can you please tell me what causing this error

Most probably its the backend api, in the given link you can see the exact state of my network calls and console.
I will also try to figure it out in case you want me to.

@Achintya-Chatterjee
Copy link
Member

@RitikJaiswal75 can you please tell me what causing this error

Most probably its the backend api, in the given link you can see the exact state of my network calls and console. I will also try to figure it out in case you want me to.

@RitikJaiswal75 I have raised a new PR and also tagged you for review. Can you please check that, you can also check whether the problem is still there or not. If it's there also I will connect with you tonight to discuss. Thank You

@Achintya-Chatterjee
Copy link
Member

This PR is moved to new PR #460

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.

Task request approval dashboard
8 participants