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

Updated mixtape embed to use bookmark card #23

Merged
merged 2 commits into from Aug 28, 2019

Conversation

rishabhgrg
Copy link
Contributor

Mixtape embed was previously using HTML card as we didn't have Bookmark card structure in place. We now have a structure for bookmark card and PR updates mixtape embed to create a bookmark card with embed information.

Bookmark Card PR - TryGhost/Ghost#11024

@rishabhgrg rishabhgrg changed the title Updated mixtape embed to use bookmark card [WIP] Updated mixtape embed to use bookmark card Aug 26, 2019
@rishabhgrg rishabhgrg force-pushed the mixtape-bookmark branch 2 times, most recently from 8a6958e to 07a9366 Compare August 27, 2019 11:59
@rishabhgrg rishabhgrg changed the title [WIP] Updated mixtape embed to use bookmark card Updated mixtape embed to use bookmark card Aug 27, 2019
@rishabhgrg rishabhgrg requested a review from ErisDS August 27, 2019 11:59
rishabhgrg and others added 2 commits August 27, 2019 15:35
no issue

We have added new bookmark card in admin which uses metadata payload to render the html for bookmark. PR updates the mixtapeEmbed to use new card with metadata instead of HTML card
@ErisDS
Copy link
Member

ErisDS commented Aug 27, 2019

Can you let me know what it is specifically that you want me to review?

@rishabhgrg
Copy link
Contributor Author

@ErisDS Just wanted to get sanity check on changes for mixtapeEmbed since you last worked on it, sorry for lack of context 😅 Its not a big change but wanted to make sure its not missing anything obvious that parser expects before merging.

@ErisDS
Copy link
Member

ErisDS commented Aug 28, 2019

Ok. As far as I can tell from the code here it's fine, but I'd be interested to know what the resulting HTML looks like now as I assume it doesn't have mixtape classes 😉

It's no big deal as I'll be using Casper which I assume has correct styling. This was only a stop gap.

@rishabhgrg
Copy link
Contributor Author

rishabhgrg commented Aug 28, 2019

@ErisDS Yeah you are right, we don't need mixtape classes anymore. The resulting html is generated from the metadata we are passing now and uses bookmark classes which is updated in Casper for styling. 😄 The html structure for bookmark looks like this -

<figure class="kg-card kg-bookmark-card">
  <a href="[URL]" class="kg-bookmark-container">
    <div class="kg-bookmark-content">
      <div class="kg-bookmark-title">[TITLE]</div>
      <div class="kg-bookmark-description">[DESCRIPTION]</div>
      <div class="kg-bookmark-metadata">
        <img src="[ICON]" class="kg-bookmark-icon">
        <span class="kg-bookmark-author">[AUTHOR]</span>
        <span class="kg-bookmark-publisher">[PUBLISHER]</span>
      </div>
    </div>
    <div class="kg-bookmark-thumbnail">
      <img src="[THUMBNAIL]">
    </div>
  </a>
</figure>

@ErisDS
Copy link
Member

ErisDS commented Aug 28, 2019

OK nice, so the bookmark card supports more fields - that's really useful to know - but nothing to change here right now.

Good to merge and ship this.

@rishabhgrg rishabhgrg merged commit abb0028 into TryGhost:master Aug 28, 2019
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