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

Enable emojis & fix migrated blog posts #68

Merged
merged 3 commits into from
Nov 17, 2020
Merged

Conversation

Ana06
Copy link
Member

@Ana06 Ana06 commented Nov 10, 2020

Enable emoji support in blog posts. 💘 😻 💚 This allows us to fix my migrated blog posts (and possibly others) as well as using emojis easily in feature blog posts. To be able to do this I needed to override the display block used for all images inside an article as it broke the plugin. IMO we shouldn't use display block for all images inside an article. This, apart from breaking jemoji (and possibly other plugins), doesn't allow the authors to decide how they want to render the images in their blog posts (for example if they want to render three images in a row). As I am not sure why this is set and where this is used and it could be a breakable change, I have kept the display block for all images by now. I have added a TODO comment and fixed it only for images with the emoji class (the ones from generated by the plugin).

I have fixed my migrated blog posts and added the license to the subtitle (it was already at the end of the blog posts) and a cover image.

Enable emoji support in blog post. This allows us to fix migrated blog
post as well as using emojis easily in feature blog posts.

Document how to use emojis
IMO we shouldn't use display block for all images inside an article. This breaks
plugins (such us jemoji) and doesn't allow the authors to decide how they
want to render the images in their blog posts (for example if they want to
render three images in a row).

As I am not sure why this is set and where this is used and it could be a
breakable change, keep the display block for all images by now. Add a
TODO comment and fix it only for images with the `emoji` class (the ones
generated by the plugin).
@Ana06 Ana06 added the enhancement New feature or request label Nov 10, 2020
@Ana06 Ana06 requested a review from hellcp November 10, 2020 22:10
@hellcp
Copy link
Member

hellcp commented Nov 10, 2020

Be mindful this breaks feeds by embedding unescaped xml in titles if they contain emoji. I wrote a crude hack to limit the impact of this for openSUSE/obs-landing@156519c

Some things got broken in my blog posts:
- Emojis: Fix them using Jemoji plugin
- Extra ###: Remove them
- Broken links: Fix them by adding the missed `https:` (only rendering
was broken)
- Deleted embedded video: Add it back

Some of these problems may be present in other blog posts as well, but there are
too many to check all of them.

I have also added the license to the subtitle (it was already at the end
of the blog posts) and a header image to all my blog posts.
@Ana06
Copy link
Member Author

Ana06 commented Nov 10, 2020

@hellcp

Be mindful this breaks feeds by embedding unescaped xml in titles if they contain emoji. I wrote a crude hack to limit the impact of this for openSUSE/obs-landing@156519c

I am not sure what you mean. I have just tried Akregator and my local version of news with an emoji in a title and it works. It renders the emoji tag and not the image, but nothing breaks. 😕

Also, what about using jekyll-feed? This way you don't need to maintain the feeds generation code.

@Ana06 Ana06 changed the title Emojis Enable emojis & fix migrated blog post Nov 11, 2020
@Ana06 Ana06 changed the title Enable emojis & fix migrated blog post Enable emojis & fix migrated blog posts Nov 11, 2020
jemoji (~> 0.12)

BUNDLED WITH
2.1.4
Copy link
Member

Choose a reason for hiding this comment

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

d10c9b5 explicitely removed the BUNDLED WITH (unfortunately without any details in the commit message).

IIRC we had some fun[tm] with it in the past because of different bundler versions in Leap and Tumbleweed (leading to "wrong version" failures), and in the end removed the BUNDLED WITH to avoid the versioned dependency. However, I'm still not familiar with ruby, so please take this with a grain of salt ;-)

Copy link
Member Author

Choose a reason for hiding this comment

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

I honestly don't understand why BUNDLED WITH was removed. It specifies the version of Bundler used to create the Gemfile.lock file. When installing the dependencies Bundler fails if your version is older than the version that created the file. This way you ensure everything works. Removing the version is IMO a bad idea, as when I install the project I don't know anymore which version I should use (and the same for production). You are removing the specification of the version dependency, but the dependency still exists and the something could break. I think we should keep BUNDLE WITH (in general we shouldn't modify this file manually) and either:

  • Update bundle in production to use a version higher or equal to the one used by the developer (or even better the newest version of bundler)
  • Document the desired version of bundle that the developers should use

Updating bundle is not a big deal and I think it is a good idea to use a newer version, so I would go for option one, except if there is a reason not to do it this way (I don't know how the deployment is done).

@Ana06
Copy link
Member Author

Ana06 commented Nov 12, 2020

@hellcp

Be mindful this breaks feeds by embedding unescaped xml in titles if they contain emoji. I wrote a crude hack to limit the impact of this for openSUSE/obs-landing@156519c

I am not sure what you mean. I have just tried Akregator and my local version of news with an emoji in a title and it works. It renders the emoji tag and not the image, but nothing breaks. 😕

@hellcp can you please elaborate on what you mean?

@hellcp
Copy link
Member

hellcp commented Nov 12, 2020

Sorry for the late response

I am not sure what you mean. I have just tried Akregator and my local version of news with an emoji in a title and it works. It renders the emoji tag and not the image, but nothing breaks. confused

I did check it and huh, well, now I wonder if that fix is even relevant anymore in obs-landing, will have to check later, thanks

Also, what about using jekyll-feed? This way you don't need to maintain the feeds generation code.

We did, it's not extensive enough, and doesn't provide everything we need. Granted a lot of our jekyll stuff is just plugin on plugin on plugin so it's no wonder ;)

@Ana06
Copy link
Member Author

Ana06 commented Nov 17, 2020

@hellcp @cboltz is there something to change then or can this be merge? 🤔

@hellcp
Copy link
Member

hellcp commented Nov 17, 2020

@cboltz would you create obs project openSUSE:infrastructure:jekyll so we can get newer bundler building so this deploys after merging?

@cboltz
Copy link
Member

cboltz commented Nov 17, 2020

@cboltz would you create obs project openSUSE:infrastructure:jekyll so we can get newer bundler building so this deploys after merging?

Done, and added permissions for you.

@hellcp
Copy link
Member

hellcp commented Nov 17, 2020

The machine is ready to take this, so LGTM

@hellcp hellcp merged commit 9e43c31 into openSUSE:master Nov 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants