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

Add the ability to set custom fonts for posts #1934

Open
wants to merge 12 commits into
base: main
Choose a base branch
from

Conversation

miniBill
Copy link
Contributor

@miniBill miniBill commented Jan 19, 2024

Security considerations: the users must not be able to create or edit fonts. I'm not sure how to enforce that.

@miniBill miniBill force-pushed the fonts branch 4 times, most recently from 472b064 to 4537ab2 Compare January 20, 2024 04:03
@miniBill miniBill marked this pull request as ready for review January 23, 2024 16:09
Copy link
Contributor

@Marri Marri left a comment

Choose a reason for hiding this comment

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

Do you want input onto how to make an admin tool to upload new fonts as well?

app/controllers/posts_controller.rb Outdated Show resolved Hide resolved
@@ -1,3 +1,7 @@
- if @fonts.present?
Copy link
Contributor

Choose a reason for hiding this comment

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

Plausibly this should go in the layout file somewhere so we're not throwing style tags in the body of the document

@@ -157,6 +157,13 @@
t.index ["post_id"], name: "index_flat_posts_on_post_id"
end

create_table "fonts", force: :cascade do |t|
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure why this didn't generate with an id: :serial? Do we need it?

expect(Setting.count).to eq(2)
expect(ContentWarning.count).to eq(2)
expect(Label.count).to eq(2)
expect(Font.count).to eq(2)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you actually want to check PostFont.count is 0 here (to make sure it failed, rather than making sure the factory worked)

expect(Setting.count).to eq(2)
expect(ContentWarning.count).to eq(2)
expect(Label.count).to eq(2)
expect(Font.count).to eq(2)
Copy link
Contributor

Choose a reason for hiding this comment

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

PostFont again

expect(Setting.count).to eq(3)
expect(ContentWarning.count).to eq(3)
expect(Label.count).to eq(3)
expect(Font.count).to eq(2)
Copy link
Contributor

Choose a reason for hiding this comment

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

PostFont again

belongs_to :post, inverse_of: :post_fonts, optional: false
belongs_to :font, inverse_of: :post_fonts, optional: false

validates :post, uniqueness: { scope: :font }
Copy link
Contributor

Choose a reason for hiding this comment

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

Unit test somewhere to ensure this?

@@ -27,6 +27,9 @@ class Post < ApplicationRecord
has_many :content_warnings, -> { ordered_by_post_tag }, through: :post_tags, source: :content_warning,
after_add: :reset_warnings, dependent: :destroy

has_many :post_fonts, dependent: :destroy
has_many :fonts, through: :post_fonts, dependent: :destroy
Copy link
Contributor

Choose a reason for hiding this comment

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

Add to a unit test somewhere ensuring this destroy happens? (to the PostFont and not the Font)

@miniBill
Copy link
Contributor Author

miniBill commented Apr 5, 2024

Do you want input onto how to make an admin tool to upload new fonts as well?

Gladly, but I'd do it in a separate PR anyway

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.

2 participants