Skip to content

Group all toggles in one component #4161

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

Open
wants to merge 24 commits into
base: development
Choose a base branch
from

Conversation

samaradel
Copy link
Contributor

@samaradel samaradel commented May 21, 2025

Description

Group all toggles in one component

Changes

Group all toggles in one component across all solutions

Related Issues

Tested Scenarios

  • Check all toggles: RentedByMe, Dedicated, and Certified if they do their job correctly

Documentation PR

For UI changes, Please provide the Documentation PR on info_grid

To consider

Preliminary Checks:

  • Preliminary Checks
    • Does it completely address the issue linked?
    • What about edge cases?
    • Does it meet the specified acceptance criteria?
    • Are there any unintended side effects?
    • Does the PR adhere to the team's coding conventions, style guides, and best practices?
    • Does it integrate well with existing features?
    • Does it impact the overall performance of the application?
    • Are there any bottlenecks or slowdowns?
    • Has it been optimized for efficiency?
    • Has it been adequately tested with unit, integration, and end-to-end tests?
    • Are there any known defects or issues?
    • Is the code well-documented?
    • Are changes to documentation reflected in the code?

UI Checks:

  • UI Checks
    • If a UI design is provided/ does it follow it?
    • Does every button work?
    • Is the data displayed logical? Is it what you expected?
    • Does every validation work?
    • Does this interface feel intuitive?
    • What about slow network? Offline?
    • What if the gridproxy/graphql/chain is failing?
    • What would a first time user have a hard time navigating here?

Code Quality Checks:

  • Code Quality Checks
    • Code formatted/linted? Are there unused imports? .. etc
    • Is there redundant/repeated code?
    • Are there conditionals that are always true or always false?
    • Can we write this more concisely?
    • Can we reuse this code? If yes, where?
    • Will the changes be easy to maintain and update in the future?
    • Can this code become too complex to understand for other devs?
    • Can this code cause future integration problems?

Testing Checklist

  • Does it Meet the specified acceptance criteria.
  • Test if changes can affect any other functionality.
  • Tested with unit, integration, and end-to-end tests.
  • Tested the un-happy path and rollback scenarios.
  • Documentation updated to meet the latest changes.
  • Check automated tests working successfully with the changes.
  • Can be covered by automated tests.

General Checklist

  • Tests included
  • Build pass
  • Documentation
  • Code format and docstring
  • Screenshots/Video attached (needed for UI changes)

Sorry, something went wrong.

samaradel added 5 commits May 21, 2025 15:36
- Fix manual import in caprover
Comment on lines 89 to 103
function onUpdateRentedByMe(val: boolean | null) {
rentedByMeModel.value = !!val;
}
function onUpdateDedicated(val: boolean | null) {
dedicatedModel.value = !!val;
}
function onUpdateCertified(val: boolean | null) {
certifiedModel.value = !!val;
}
function onUpdateHasGPU(val: boolean | null) {
hasGPUModel.value = !!val;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

i think inline change the values in template would be better, Those founciton are not used anywhere else
example :

:model-value="(val)=> gpuModule = val"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did, but it caused type-check errors, let me try adding types there

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@0oM4R I tried it, but it caused the same error I mentioned, so I returned to the previous solution.

Copy link
Contributor

Choose a reason for hiding this comment

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

  <v-switch
      color="primary"
      inset
      label="Rented By Me"
      :model-value="rentedByMeModel"
      hide-details
      @update:model-value="(val) => rentedByMeModel = !!val"
    />

this works fine with me without any errors

Copy link
Contributor

Choose a reason for hiding this comment

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

@samaradel please check this comment

@samaradel samaradel marked this pull request as draft May 22, 2025 13:46
samaradel added 3 commits May 22, 2025 17:02
@samaradel samaradel marked this pull request as ready for review May 22, 2025 14:28
@samaradel samaradel requested a review from 0oM4R May 22, 2025 14:30
@samaradel samaradel marked this pull request as draft May 25, 2025 08:54
samaradel added 2 commits May 28, 2025 08:34

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
@samaradel samaradel marked this pull request as ready for review May 28, 2025 05:39
@0oM4R
Copy link
Contributor

0oM4R commented May 28, 2025

image

after toggle some of mentioned toggles, it is stuck in loading, and the pages in the response look weird!

@samaradel samaradel marked this pull request as draft June 1, 2025 09:24
@samaradel samaradel requested a review from 0oM4R June 1, 2025 09:49
@samaradel samaradel marked this pull request as draft June 3, 2025 13:25
@samaradel samaradel marked this pull request as ready for review June 3, 2025 13:58
samaradel added 2 commits June 4, 2025 11:32
Copy link
Contributor

@0oM4R 0oM4R left a comment

Choose a reason for hiding this comment

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

there are many unrelated changes, please make sure to pull development and run eslint

Screenshot from 2025-06-10 12-02-43

@samaradel samaradel requested a review from 0oM4R June 12, 2025 07:59
Copy link
Contributor

@0oM4R 0oM4R left a comment

Choose a reason for hiding this comment

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

i still see many unrelated changes please make sure to restore the original files from development

Comment on lines 89 to 103
function onUpdateRentedByMe(val: boolean | null) {
rentedByMeModel.value = !!val;
}
function onUpdateDedicated(val: boolean | null) {
dedicatedModel.value = !!val;
}
function onUpdateCertified(val: boolean | null) {
certifiedModel.value = !!val;
}
function onUpdateHasGPU(val: boolean | null) {
hasGPUModel.value = !!val;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

@samaradel please check this comment

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
@samaradel
Copy link
Contributor Author

@0oM4R already did, I merged the development branch and ran yarn lint

@samaradel samaradel requested a review from 0oM4R June 12, 2025 13:10
Copy link
Contributor

@0oM4R 0oM4R left a comment

Choose a reason for hiding this comment

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

@samaradel
you can restore the unrelated changes by
git checkout origin/development <pathToTheFile>
This will get the version that is on the development branch

also please make sure to check the other comments

@samaradel samaradel marked this pull request as draft June 18, 2025 05:37
@samaradel samaradel marked this pull request as ready for review June 18, 2025 06:45
@samaradel
Copy link
Contributor Author

@0oM4R I did, and it's something in linting rules, as it keeps changing when I commit

@samaradel samaradel requested a review from 0oM4R June 18, 2025 06:48

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
@0oM4R
Copy link
Contributor

0oM4R commented Jun 18, 2025

so this is due to the pre-commit script, as it sees the unrelated files you just reverted as changed files then apply liniting on it
so the solution is on commit
`git commit -n -m "commit message"

feel free to reach me out if any help needed

@samaradel samaradel marked this pull request as draft June 22, 2025 08:22
@samaradel samaradel marked this pull request as ready for review June 22, 2025 08:49
@samaradel samaradel marked this pull request as draft July 10, 2025 07:11
@samaradel samaradel marked this pull request as ready for review July 10, 2025 07:37
Copy link
Contributor

@0oM4R 0oM4R left a comment

Choose a reason for hiding this comment

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

Please check if @amiraabouhadid still has any comments

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.

None yet

3 participants