-
Notifications
You must be signed in to change notification settings - Fork 508
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
AO3-6464 Allow ruby, rt, and rp HTML tags where HTML is allowed #4443
Changes from 1 commit
ff2b764
41b95bd
e420617
65a138d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -85,4 +85,9 @@ | |
word_counter.text = "\“嘿Bob,\” Alice说,‘啊?!?’" | ||
expect(word_counter.count).to eq(5) | ||
end | ||
|
||
it "doesn't count parentheses in rp" do | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You can leave out this test, which is not too related to the current issue (the word counter ignores all HTML tags). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's true that this is not a test that justifies the other changes in the PR. It would have passed before. So I agree with leaving it out if that's what you mean. There doesn't seem to be tests that involve parenthesized text, full-width or not, and parentheses might be common things that would warrant tests, but it seems like that's a separate issue to think about. (That's a long way to say "yes" but I wanted to make sure I have it right.) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, let's leave this alone for now as it's not related to the main changes in the PR. |
||
word_counter.text = "<ruby>日本語<rp>(</rp><rt>にほんご</rt><rp>)</rp></ruby>" | ||
expect(word_counter.count).to eq(7) | ||
end | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's another list at
otwarchive/app/helpers/application_helper.rb
Lines 67 to 74 in 2d92ec1
but I don't think we ever set
show_list = true
anywhere. Can you remove that option and the conditional that uses it?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For all I could find, it was disabled in 1d9f656 (in 2011).
The option is used in app/views/bookmarks/_bookmark_form.html.erb and app/views/comments/_comment_form.html.erb (with show_list=false). I feel like I would be touching parts of the code that's not particularly related to the issue (ruby annotations), but if that's okay, sure, I'll do that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's fine to make these changes. It's unused code that looks puzzling if not updated to include to new tags.