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

[docs] Merge ImageSpotlight and Video to ContentSpotlight #29641

Merged

Conversation

amandeepmittal
Copy link
Member

Why

Closes ENG-11611

How

  • Use a single React component (ContentSpotlight) to share props for displaying an image or a video in the Expo docs.
  • All functionality and props are the same from ImageSpotlight and Video.
  • Convert and remove emotion/css to Tailwind for displaying a Video in this new component.

Test Plan

By running docs locally, replacing ImageSpotlight and Video references with ContentSpotlight.

Preview of tested changes

CleanShot 2024-06-11 at 16 23 44@2x

CleanShot 2024-06-11 at 16 23 38@2x

CleanShot 2024-06-11 at 16 23 32@2x

Checklist

Copy link

linear bot commented Jun 11, 2024

@expo-bot expo-bot added the bot: passed checks ExpoBot has nothing to complain about label Jun 11, 2024
Copy link
Collaborator

@Simek Simek left a comment

Choose a reason for hiding this comment

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

Looks good! Left few comments mostly related to new component styling code.

docs/ui/components/ContentSpotlight/index.tsx Outdated Show resolved Hide resolved
docs/ui/components/ContentSpotlight/index.tsx Outdated Show resolved Hide resolved
docs/ui/components/ContentSpotlight/index.tsx Outdated Show resolved Hide resolved
docs/ui/components/ContentSpotlight/index.tsx Outdated Show resolved Hide resolved
docs/ui/components/ContentSpotlight/index.tsx Outdated Show resolved Hide resolved
docs/ui/components/ContentSpotlight/index.tsx Outdated Show resolved Hide resolved
docs/ui/components/ContentSpotlight/index.tsx Outdated Show resolved Hide resolved
docs/ui/components/ContentSpotlight/index.tsx Outdated Show resolved Hide resolved
docs/ui/components/ContentSpotlight/index.tsx Outdated Show resolved Hide resolved
docs/ui/components/ContentSpotlight/index.tsx Outdated Show resolved Hide resolved
Copy link
Collaborator

@Simek Simek left a comment

Choose a reason for hiding this comment

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

Tested the now component locally, looks and works nice, thanks! 👍

@amandeepmittal amandeepmittal merged commit 75ab2e2 into main Jun 14, 2024
4 checks passed
@amandeepmittal amandeepmittal deleted the aman/eng-11611-unify-imagespotlight-and-video-components branch June 14, 2024 13:22
amandeepmittal added a commit that referenced this pull request Jun 16, 2024
…light` (#29787)

# Why

<!--
Please describe the motivation for this PR, and link to relevant GitHub
issues, forums posts, or feature requests.
-->
Closes ENG-12469 ENG-12468

# How

<!--
How did you build this feature or fix this bug and why?
-->
- Use `ContentSpotlight` to display images and videos in our docs and
replace `ImageSpotlight` and `Video` components.
- Remove `ImageSpotlight` and `Video` component files.
- Update **docs/README.md** > Add videos section's example to use
`ContentSpotlight`.
- Follow up #29641

# Test Plan

<!--
Please describe how you tested this change and how a reviewer could
reproduce your test, especially if this PR does not include automated
tests! If possible, please also provide terminal output and/or
screenshots demonstrating your test/reproduction.
-->

Run docs locally and manually check that all images and videos are
displayed as usual. Also run `yarn run test` and all tests are passing.

# Checklist

<!--
Please check the appropriate items below if they apply to your diff.
This is required for changes to Expo modules.
-->

- [x] Documentation is up to date to reflect these changes (eg:
https://docs.expo.dev and README.md).
- [ ] Conforms with the [Documentation Writing Style
Guide](https://github.com/expo/expo/blob/main/guides/Expo%20Documentation%20Writing%20Style%20Guide.md)
- [ ] This diff will work correctly for `npx expo prebuild` & EAS Build
(eg: updated a module plugin).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bot: passed checks ExpoBot has nothing to complain about
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants