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.

Sign up for GitHub

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

Jump to bottom

Add Event Block #186

Merged
merged 1 commit into from
Mar 1, 2024
Merged

Add Event Block #186

merged 1 commit into from
Mar 1, 2024

Conversation

conrad-vanl
Copy link
Member

@conrad-vanl conrad-vanl commented Feb 29, 2024 •
edited
Loading

🐛 Issue

No event block in web, part of cycle goals

✏️ Solution

Add event block

Area.mp4

🔬 To Test

  1. On staging admin create a content item with an event block
  2. While pointed at staging, open that content item in the web service

📸 Screenshots

CleanShot 2024-02-29 at 16 07 09@2x

CleanShot 2024-02-29 at 16 07 38@2x

@vercel Vercel
Copy link

vercel bot commented Feb 29, 2024 •
edited
Loading

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

Name Status Preview Comments Updated (UTC)
apollos-micro-service ✅ Ready (Inspect) Visit Preview 💬 Add feedback Mar 1, 2024 7:47pm
apollos-web-embeds ✅ Ready (Inspect) Visit Preview 💬 Add feedback Mar 1, 2024 7:47pm

@conrad-vanl Graphite App
Copy link
Member Author

conrad-vanl commented Feb 29, 2024 •
edited
Loading

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @conrad-vanl and the rest of your teammates on Graphite Graphite

@conrad-vanl conrad-vanl mentioned this pull request Feb 29, 2024
Merged
@conrad-vanl conrad-vanl marked this pull request as ready for review February 29, 2024 22:08
@conrad-vanl conrad-vanl force-pushed the 02-29-Add_Event_Block branch from 4e150b4 to 8bbe2ec Compare March 1, 2024 03:51
@nlewis84
Copy link
Contributor

nlewis84 commented Mar 1, 2024

Should have just used pentagram for Outlook. Missed opportunity.

nlewis84
nlewis84 reviewed Mar 1, 2024
View reviewed changes
Copy link
Contributor

@nlewis84 nlewis84 left a comment

Choose a reason for hiding this comment

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

I'm having trouble testing this. No event blocks coming through. 🫤

Code looks good, and thanks for the video. Do you have a link to an item you are using for testing....maybe I can just look at it there? Maybe drop the link to the Admin item and the micro-service item in the description?

@conrad-vanl
Copy link
Member Author

@nlewis84 are you running https://github.com/ApollosProject/apollos-cluster/pull/917 locally and pointing embeds at that?

@conrad-vanl
Copy link
Member Author

Forgot to list that as requirement. And that has to be merged and released first anyways

@nlewis84
Copy link
Contributor

nlewis84 commented Mar 1, 2024

Ah nope, I wasn't. That would do it.

Base automatically changed from 02-29-Upgrade_phosphor to main March 1, 2024 18:07
@nlewis84
Copy link
Contributor

nlewis84 commented Mar 1, 2024

These three event blocks (something in there) gives me the following error in web-embeds:
Screenshot 2024-03-01 at 12 53 58 PM
Screenshot 2024-03-01 at 12 54 10 PM

@nlewis84
Copy link
Contributor

nlewis84 commented Mar 1, 2024

Probably that text for multi line location. Should we have validation in Admin?

@nlewis84
Copy link
Contributor

nlewis84 commented Mar 1, 2024

Hmmm, yeah a text location (think like "Virtual" or "Spring Creek Campus") will break the convertToIcsLink and convertAddressToGoogleMapLink functions. With some selective commenting, I can get them to show up and they look like this. Any adjustments you would like to make for styling?

Also, there should be a title on that 3rd one but it doesn't come through.

Screenshot 2024-03-01 at 1 08 17 PM

@conrad-vanl Graphite App
Copy link
Member Author

@nlewis84 just pushed up some changes see if that helps

@conrad-vanl conrad-vanl force-pushed the 02-29-Add_Event_Block branch from 8bbe2ec to 980a3ca Compare March 1, 2024 19:42
@nlewis84 nlewis84 self-requested a review March 1, 2024 19:51
nlewis84
nlewis84 approved these changes Mar 1, 2024
View reviewed changes
Copy link
Contributor

@nlewis84 nlewis84 left a comment

Choose a reason for hiding this comment

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

One design comment, but working and looks good to me otherwise!

packages/web-shared/components/AddToCalendar/AddToCalendar.styles.js
color: ${themeGet('colors.text.primary')};
border-radius: ${themeGet('radii.xl')};
background-color: ${themeGet('colors.fill.paper')};
box-shadow: ${themeGet('shadows.medium')};
Copy link
Contributor

Choose a reason for hiding this comment

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

Probably this one too 🤷‍♂️

packages/web-shared/components/FeatureFeed/Features/EventBlockFeature.styles.js
color: ${themeGet('colors.text.primary')};
border-radius: ${themeGet('radii.xl')};
background-color: ${themeGet('colors.fill.paper')};
box-shadow: ${themeGet('shadows.medium')};
Copy link
Contributor

Choose a reason for hiding this comment

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

This shadow seems excessive to me, but I don't like the low option either
Screenshot 2024-03-01 at 1 55 07 PM

Admin's shadow is much more subtle
Screenshot 2024-03-01 at 1 56 35 PM

@conrad-vanl Graphite App
Copy link
Member Author

yeah i agree, but i didn't like the other shadows either and didn't want to define a new one

@conrad-vanl conrad-vanl merged commit aba5a64 into main Mar 1, 2024
6 checks passed
@conrad-vanl conrad-vanl deleted the 02-29-Add_Event_Block branch March 1, 2024 20:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nlewis84 nlewis84 nlewis84 approved these changes

@vinnyjth vinnyjth Awaiting requested review from vinnyjth

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@conrad-vanl @nlewis84