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

[REQUEST] #54 : Added a hard coded backlit color for story images. #57

Merged
merged 4 commits into from
Oct 11, 2024

Conversation

PRIME-SV
Copy link
Contributor

Describe your changes

  • Added the hardcoded backlit color to the images that doesn't fill the story view.
  • Aligned the landscape img/video to the center.

Screenshots - If Any (Optional)

image
image
image

Issue ticket number and link - If Any

Checklist before requesting a review

  • I have performed a self-review of my code.

  • Followed the repository's Contributing Guidelines.

  • I ran the app and tested it locally to verify that it works as expected.

  • I have starred the repository.

Additional Information (Optional)

  • For now even though the image/video is portrait, it is getting center align.
  • For "Fallback: In cases where the effect may not be supported (e.g., older browsers or devices), provide a graceful fallback where the effect is disabled without breaking the story display." I could not reproduce the scenario so not tested.

Copy link

vercel bot commented Oct 10, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
ngx-stories ✅ Ready (Inspect) Visit Preview 💬 Add feedback Oct 11, 2024 1:15pm

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Hey @PRIME-SV, Thanks for contributing and Congrats on opening PR 🎉.

We will try to review as soon as possible and a maintainer will get back to you soon!

@Gauravdarkslayer
Copy link
Owner

@PRIME-SV , Can we allow background color property to be a @input() property ?
Like in a global configuration or per story as well.
By default a black color(#1B1B1B) should work here .
Then in the next phase we will allow dynamic background coloring as well.

Apart from this i've noticed that the height became larger then usual, can you please have a look at that as well ?
react-insta-stories
Checkout this library in react they have a default width and height to not over stretch the story view.

@PRIME-SV
Copy link
Contributor Author

Hi @Gauravdarkslayer ,
Added backlitColor as a Input() property and assigned a default value of #1b1b1b.
Also for height being larger than before, I think it seems so because of the stretched ss 😁. I am not able to get it unstretch on my Windows.
As I can see the height is the same as referred in react library.

@Gauravdarkslayer
Copy link
Owner

@PRIME-SV In the main app.component.ts let's add #1b1b1b for now instead of lightGray color.
Also can you try to adjust the height so that it will remain in the view i.e. without scrolling.
As it looks like the margin from top or bottom is little too more.

@PRIME-SV
Copy link
Contributor Author

Sure.
Changed the color.
This is how it looks on desktop.
image

On Mobile:
image

@Gauravdarkslayer
Copy link
Owner

Looks like video stories still have white background.

@PRIME-SV
Copy link
Contributor Author

In the Request Description,
Drawback section it says "In case of videos based story we can ignore this implementation". So implemented this for images only.

@Gauravdarkslayer
Copy link
Owner

So it was for dynamic backlit, which is depends on the image/video itself just like we see on Instagram, it takes image/video color only to generate the backlit color, but for now we're doing this directly so I think this won't be challenging to add color right?

@PRIME-SV
Copy link
Contributor Author

PRIME-SV commented Oct 11, 2024

Sure its not that challenging.
My bad, maybe I misunderstood the requirement.
I will update the same for video.

@PRIME-SV
Copy link
Contributor Author

Updated the code.

Copy link
Owner

@Gauravdarkslayer Gauravdarkslayer left a comment

Choose a reason for hiding this comment

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

Sure thanks for this valuable contribution !

@Gauravdarkslayer Gauravdarkslayer merged commit 9bdfb8f into Gauravdarkslayer:main Oct 11, 2024
4 checks passed
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.

2 participants