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

verticalList || ImageGallery buttons issue #4099 solved #4105

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

Conversation

sanwalsulehri
Copy link

@sanwalsulehri sanwalsulehri commented Jan 23, 2025

PR Checklist

PR Type

What kind of change does this PR introduce?

  • Bugfix (fixes an issue)
  • Feature (adds functionality)
  • Code style update
  • Refactoring (no functional changes)
  • Build related changes
  • CI related changes
  • Documentation changes
  • Other... Please describe:

What is the current behavior?

What is the new behavior?

Describe the new behaviour
If useful, provide screenshot or capture to highlight main changes

Does this PR introduce a DB Schema Change or Migration?

  • Yes
  • No

Git Issues

Closes #4099

What happens next?

Thanks for the contribution! We try to make sure all PRs are reviewed ahead of our monthly maintainers call (first Monday of the month)

If the PR is working as intended it'll be merged and included in the next platform release, if not changes will be requested and re-reviewed once updated.

If you need more immediate feedback you can try reaching out on Discord in the Community Platform development channel.

@sanwalsulehri sanwalsulehri requested a review from a team as a code owner January 23, 2025 22:12
@mariojsnunes
Copy link
Contributor

@sanwalsulehri the pipeline is failing due to code not formatted. you should be able to fix that by running yarn format.
To avoid this in the future, make sure you have the prettier extension installed and "format on save" enabled :)

image

@sanwalsulehri
Copy link
Author

ok nw i am done this

@sanwalsulehri
Copy link
Author

hey @mariojsnunes thanks for guide and now see man

Copy link

cypress bot commented Jan 23, 2025

onearmy-community-platform    Run #6824

Run Properties:  status check passed Passed #6824  •  git commit 05b108c759: verticalList || ImageGallery buttons issue #4099 solved
Project onearmy-community-platform
Branch Review pull/4105
Run status status check passed Passed #6824
Run duration 05m 28s
Commit git commit 05b108c759: verticalList || ImageGallery buttons issue #4099 solved
Committer sanwalsulehri
View all properties for this run ↗︎

Test results
Tests that failed  Failures 0
Tests that were flaky  Flaky 3
Tests that did not run due to a developer annotating a test with .skip  Pending 0
Tests that did not run due to a failure in a mocha hook  Skipped 0
Tests that passed  Passing 79
View all changes introduced in this branch ↗︎

}

/* Set red background for PhotoSwipe navigation buttons */
.pswp__button--arrow--prev {
Copy link
Contributor

Choose a reason for hiding this comment

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

are all these !important necessary?

Copy link
Author

Choose a reason for hiding this comment

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

yes actually

Copy link
Author

Choose a reason for hiding this comment

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

the styling not applied that's why man everything checked no failure you see

Copy link
Author

Choose a reason for hiding this comment

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

@mariojsnunes everything passed mean checked now no failure that's a good news

Copy link
Author

Choose a reason for hiding this comment

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

@mariojsnunes i think now this issue are solved so plz merge and closed this issue😃🎉

Copy link
Author

Choose a reason for hiding this comment

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

any help you want in code or remove anything or solve any issue feel free to tell me i will solved this @mariojsnunes

@sanwalsulehri
Copy link
Author

why not you merged this code guys @mariojsnunes plz tell me if you wnat any change in code or somewhere i do but merged this code plz

@mariojsnunes
Copy link
Contributor

why not you merged this code guys @mariojsnunes plz tell me if you wnat any change in code or somewhere i do but merged this code plz

@sanwalsulehri please be patient, we are not working on this fulltime, it could even take a few days. I'd like for @benfurber to review this as well.

@sanwalsulehri
Copy link
Author

ok thanks @mariojsnunes and sorry for this also😃😃

@benfurber
Copy link
Member

Hey @sanwalsulehri, thanks for your first contribution!

I think I'm looking for a different approach to this ticket.

The issue asks for a new component, for us that means in the component library probably called something like ScrollViewArrow. That can then replace the styling found on packages/components/src/VerticalList/Arrows.tsx and the NavButton you've found. A simple example of a new component is the Accordion here. Rather than css, our default approach is the sx prop on top of theme-ui components like Flex or Box , we have done some vanilla css recently but this is one I'm confident can be done with sx.

Make sense?

@benfurber
Copy link
Member

Also, it would be super helpful if you could add screenshots of the change to the PR description. :)

@sanwalsulehri
Copy link
Author

Ok i will try but not for now tomoroow i wil do this easily bcz i am on an function

@benfurber
Copy link
Member

Awesome, thanks @sanwalsulehri

@sanwalsulehri
Copy link
Author

Hi @benfurber i try through sx={{}} but its not working you get the layout the !important are not harmfull for your code and this is a little css code that are absolutely not harmfull for your code so don't worry and plz merged bcz the sx={{}} are not working🙌

@benfurber
Copy link
Member

@sanwalsulehri If you start by editing this props, you'll work it out:
Screenshot 2025-01-27 at 10 52 09

@sanwalsulehri
Copy link
Author

@sanwalsulehri
Copy link
Author

@benfurber that's why i am using css you use a little bit css in your project so their is no changes created if we ad 5 more lines of css!!!!!

@sanwalsulehri
Copy link
Author

@benfurber brother i think css is good for this if you have anyother idea plz tell me i will try the idea also

@benfurber
Copy link
Member

@sanwalsulehri If you were editing the sx props from this code block, I'm not suprised this doesn't show a change as that's a different component. VerticalList is for the arrows on the map page and the research/library/questions index pages.

@sanwalsulehri
Copy link
Author

sanwalsulehri commented Jan 29, 2025

ok i got it can you tell me in which component these arrows are if you know plz tell me bcz its too much helpfull for me @benfurber

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: 💬 Changes Requested/with author
Development

Successfully merging this pull request may close these issues.

[ui] One type of left/right style buttons
3 participants