It’s the fifth week of Hacktoberfest and we are continuing to highlight the amazing people in our community who have contributed to Forem's codebase!
We’re thrilled to share that we’ve had 10 contributors this week commit improvements across the many projects we have in our web, iOS, and Android apps 🎉
Thanks to the help of these wonderful folks, we’ve merged 15 PRs this past week. Forem is still a small team so this amount of feature-building and bug-squashing is truly only possible because of the community. As Forem grows, we intend to continue enabling the open source ecosystem to improve and expand our offerings with their invaluable help. We appreciate everyone for helping us sow this open source commitment from day one.
If you are interested in contributing, check out our post on Forem projects you can contribute to this Hacktoberfest:
Contribute to Forem this Hacktoberfest!
Christina Gorton for The DEV Team ・ Oct 2 '20
In no particular order, here are the folks who made commits this week, their GitHub profiles, and their merged PRs.
Fix reading list counter #10763
What type of PR is this? (check all applicable)
- [ ] Refactor
- [ ] Feature
- [x] Bug Fix
- [ ] Optimization
- [ ] Documentation Update
Description
I found that a wrong reading list count was caused by caching. The cache of cached_reading_list_article_ids is changed only when public_reactions_count
is updated, but we must also recache it when user archives and unarchives an article.
To achieve this, I added the last_changed_archive_at
column to the users table and cache keys.
I could not come up with an idea to fix this without adding an extra column. Will you let me know If you have better thoughts, I'll give it a try.
Related Tickets & Documents
Closes https://github.com/forem/forem/issues/9770
QA Instructions, Screenshots, Recordings
Please replace this line with instructions on how to test your changes, as well as any relevant images for UI changes.
Added tests?
- [x] yes
- [ ] no, because they aren't needed
- [ ] no, because I need help
Added to documentation?
- [ ] docs.forem.com
- [ ] readme
- [x] no documentation needed
[optional] Are there any post deployment tasks we need to perform?
[optional] What gif best describes this PR or how it makes you feel?
Fix grammar error in Code Climate #10935
What type of PR is this? (check all applicable)
- [ ] Refactor
- [ ] Feature
- [ ] Bug Fix
- [ ] Optimization
- [x] Documentation Update
Description
Combined two sentences in the last paragraph of Code Climate to fix sentence fragments.
Added tests?
- [ ] yes
- [x] no, because they aren't needed
- [ ] no, because I need help
Added to documentation?
- [x] docs.forem.com
- [ ] readme
- [ ] no documentation needed
Document overriding mailer templates #10574
What type of PR is this? (check all applicable)
- [ ] Refactor
- [ ] Feature
- [ ] Bug Fix
- [ ] Optimization
- [x] Documentation Update
Description
Updates documentation relating to emails. In particular, documents how the mailer views can be overridden, especially for devise_invitable
.
While working on this, I identified that there aren't mailer previews for devise_invitable
(#10575).
Related Tickets & Documents
#9849
QA Instructions, Screenshots, Recordings
Edit the contents of a mailer view under the given directories, and check the mailer previews to make sure they make sense.
Added tests?
- [ ] yes
- [x] no, because they aren't needed
- [ ] no, because I need help
Added to documentation?
- [x] docs.forem.com
- [ ] readme
- [ ] no documentation needed
Adding file extensions to gitattributes #10945
What type of PR is this?
- [ ] Refactor
- [ ] Feature
- [ ] Bug Fix
- [x] Optimization
- [ ] Documentation Update
Description
Adding file extensions to .gitattributes for better diff. It is mostly inspired from https://tekin.co.uk/2020/10/better-git-diff-output-for-ruby-python-elixir-and-more
Related Tickets & Documents
Closes https://github.com/forem/forem/issues/10946
QA Instructions, Screenshots, Recordings
Try doing git diff using git cli before and after this commit on a ruby file
Added tests?
- [ ] yes
- [x] no, because they aren't needed
- [ ] no, because I need help
Added to documentation?
- [ ] docs.forem.com
- [ ] readme
- [x] no documentation needed
&
Removing hard-coded email #10125
What type of PR is this?
- [ ] Refactor
- [ ] Feature
- [x] Bug Fix
- [ ] Optimization
- [ ] Documentation Update
Description
Removing hardcoded email yo@dev.to
Related Tickets & Documents
Closes https://github.com/forem/forem/issues/9826
Added tests?
- [x] yes
- [ ] no, because they aren't needed
- [ ] no, because I need help
Added to documentation?
- [ ] docs.forem.com
- [ ] readme
- [x] no documentation needed
What gif best describes this PR or how it makes you feel?
Avoid CSS word wrap in video player duration #10889
What type of PR is this? (check all applicable)
- [ ] Refactor
- [ ] Feature
- [x] Bug Fix
- [ ] Optimization
- [ ] Documentation Update
Description
These new CSS rules override the overflow-wrap: anywhere
and word-break: break-word
rules in .crayons-article__header
and also the overflow-wrap: anywhere
rule in .crayons-card
, but only for the JW player part of the DOM.
Related Tickets & Documents
Closes #10799
QA Instructions, Screenshots, Recordings
Added tests?
- [ ] yes
- [x] no, because they aren't needed
- [ ] no, because I need help
Added to documentation?
- [ ] docs.forem.com
- [ ] readme
- [x] no documentation needed
&
Preserve newlines and indentation in <pre> tags from RSS import #10922
What type of PR is this? (check all applicable)
- [ ] Refactor
- [ ] Feature
- [x] Bug Fix
- [ ] Optimization
- [ ] Documentation Update
Description
I tracked down the issue to the reverse_markdown
gem and found this unmerged fix: https://github.com/xijo/reverse_markdown/pull/78. Since it seems we are already overriding the <pre>
converter I went ahead and added the fix to our CustomPre
implementation, together with a simple spec.
Related Tickets & Documents
Closes #10104
QA Instructions, Screenshots, Recordings
I tested using the sample RSS feed provided in the issue.
The easiest way I found for me to test the feed parsing was to create a draft Article and then from rails console
perform an update with the code from RssReader
:
feed = Feedjira.parse(File.read('sample_rss.xml'))
Article.last.update(
body_markdown: RssReader::Assembler.call(feed.entries.first, User.find(4), feed, feed.entries.first.url.strip.split("?source=")[0])
)
Added tests?
- [x] yes
- [ ] no, because they aren't needed
- [ ] no, because I need help
Added to documentation?
- [ ] docs.forem.com
- [ ] readme
- [x] no documentation needed
&
Fix RSpec unit testing when using Docker dev setup #10950
What type of PR is this? (check all applicable)
- [ ] Refactor
- [ ] Feature
- [x] Bug Fix
- [ ] Optimization
- [ ] Documentation Update
Description
When using the Docker dev setup, Elasticsearch is available at elasticsearch:9200
instead of localhost:9200
so the VCR testing config was not ignoring the requests needed for RSpec setup when doing Search::Cluster.recreate_indexes
in spec/rails_helper.rb
. Also the WebMock setup allows localhost
requests which should also include Elasticsearch requests.
This seems okay to me, but I'm not sure if there is a better solution. One issue I might have is that a detail related to the Docker dev setup is bleeding into common RSpec configs. What do you think, @citizen428?
Related Tickets & Documents
Closes #10579
QA Instructions, Screenshots, Recordings
With this change you can run unit tests when using the Docker dev setup, for example this spec was failing at Search::Cluster.recreate_indexes
in spec/rails_helper.rb
and is now passing.
bundle exec rspec spec/liquid_tags/vimeo_tag_spec.rb
Added tests?
- [ ] yes
- [x] no, because they aren't needed
- [ ] no, because I need help
Added to documentation?
- [ ] docs.forem.com
- [ ] readme
- [x] no documentation needed
&
Use matched substring in Spotify liquid tag to avoid trailing whitespace #10953
What type of PR is this? (check all applicable)
- [ ] Refactor
- [ ] Feature
- [x] Bug Fix
- [ ] Optimization
- [ ] Documentation Update
Description
The bug was caused by a trailing whitespace character in the embed URL. Using the matched substring from MatchData
avoids any additional whitespace. Also updated the spec to use explicit values instead of duplicating the logic from the code.
Related Tickets & Documents
Closes #10951
QA Instructions, Screenshots, Recordings
Adding a Spotify liquid tag to an Article will now result in a valid embed iframe, while before it would display the text "Bad Request".
For example:
{% spotify spotify:track:3GfOAdcoc3X5GPiiXmpBjK %}
Added tests?
- [x] yes
- [ ] no, because they aren't needed
- [ ] no, because I need help
Added to documentation?
- [ ] docs.forem.com
- [ ] readme
- [x] no documentation needed
Add aliases for mod_roundrobin_notifications + email_follower_notifications to user model #6892 #10892
What type of PR is this? (check all applicable)
- [ ] Refactor
- [x] Feature
- [ ] Bug Fix
- [ ] Optimization
- [ ] Documentation Update
Description
Related Tickets & Documents
Closes #6892
QA Instructions, Screenshots, Recordings
n/a
Added tests?
- [x] no, because they aren't needed
Added to documentation?
- [x] no documentation needed
Mod Center: clicking on article title opens a in new tab #9314
What type of PR is this? (check all applicable)
- [ ] Refactor
- [ ] Feature
- [x] Bug Fix
- [ ] Optimization
- [ ] Documentation Update
Description
Was able to create a clickable title on the modCenter page where clicking on the article title would open a new tab to the the article. Clicking on the label will bring up the original inline article mod.
Related Tickets & Documents
Closes https://github.com/forem/forem/issues/8976
I was also able to get a clickable link on the author to work, which seemed to have no errors on my local host end. However, when trying to commit and run the test cases on singleArticle.text.jsx, it wasn't able to pass one of the test on line 73 to 77 where it renders the author's name. I have commented the code out if anyone can look and see what the problem is.
I also wasn't able to implement @jacobherrington suggestion of having a external link icon simply because I wasn't able to import the externalicon.svg, maybe it's connect to the crayon system?
QA Instructions, Screenshots, Recordings
Added tests?
- [x] yes
- [ ] no, because they aren't needed
- [ ] no, because I need help
Added to documentation?
- [ ] docs.dev.to
- [ ] readme
- [x] no documentation needed
refactor: Use keyboard shortcut hook in article form #11014
What type of PR is this? (check all applicable)
- [x] Refactor
- [ ] Feature
- [ ] Bug Fix
- [ ] Optimization
- [ ] Documentation Update
Description
Now we have a helper for keyboard shortcuts we can move old shortcuts over to this new method
Related Tickets & Documents
#10878
QA Instructions, Screenshots, Recordings
Press ctrl/command + shift + p in the new post page it should toggle the preview
Added tests?
- [ ] yes
- [x] no, because they aren't needed
- [ ] no, because I need help
Added to documentation?
- [ ] docs.forem.com
- [ ] readme
- [x] no documentation needed
[optional] Are there any post deployment tasks we need to perform?
N/A
[optional] What gif best describes this PR or how it makes you feel?
&
Allow keyboard shortcuts #10713
What type of PR is this? (check all applicable)
- [ ] Refactor
- [x] Feature
- [ ] Bug Fix
- [ ] Optimization
- [ ] Documentation Update
Description
Add handler for keyboard shortcuts. This allows custom shortcuts to be set up on a page by page (or even component by component) basis.
Usage
In this example we're going to run a function when the user presses CTRL+SHIFT+P
const shortcuts = {
"ctrl+shift+KeyP": (e) => {
e.preventDefault();
someFunction();
}
}
<KeyboardShortcuts shortcuts={shortcuts} />
Related Tickets & Documents
related: #5023 #596
QA Instructions, Screenshots, Recordings
This facilitates changes in the future
Added tests?
- [x] yes
- [ ] no, because they aren't needed
- [ ] no, because I need help
Added to documentation?
- [ ] docs.forem.com
- [ ] readme
- [x] no documentation needed
[optional] Are there any post deployment tasks we need to perform?
Once this is merged I'd like to modify this current shortcut to use this method before thinking about what other shortcuts could/should be added.
[optional] What gif best describes this PR or how it makes you feel?
refactor: Use keyboard shortcut hook in listing modal #11017
What type of PR is this? (check all applicable)
- [x] Refactor
- [ ] Feature
- [ ] Bug Fix
- [ ] Optimization
- [ ] Documentation Update
Description
Use the new keyboard shortcut hook to listen for Escape
keypress to close an open listing modal
Related Tickets & Documents
#10878
QA Instructions, Screenshots, Recordings
- Go to listings
- Open a listing
- Press
Escape
- Verify that the listing modal closes
Added tests?
- [ ] yes
- [ ] no, because they aren't needed
- [x] no, because I need help
I'd like to add tests for this, but the <Listings />
component does not currently have tests.
It seems to need a lot of stuff in order to set it up (mocking, adding divs to the DOM, etc.).
Any help with that is appreciated.
Closing the modal is probably not the most crucial test to do in this component, but if making the set up opens the door to more tests, then it's a win.
Added to documentation?
- [ ] docs.forem.com
- [ ] readme
- [x] no documentation needed
What gif best describes this PR or how it makes you feel?
iOS
Roman Podymov
A small refactoring #235
What type of PR is this? (check all applicable)
- [x] Refactor
- [ ] Feature
- [ ] Bug Fix
- [ ] Documentation Update
Description
Hello. Thank you for DEV-ios. I did a small refactoring in MediaManager.swift.
Related Tickets & Documents
Mobile & Desktop Screenshots/Recordings (if there are UI changes)
Added to documentation?
- [ ] docs.dev.to
- [ ] readme
- [x] no documentation needed
Thanks again for improving both Forem and the broader open source community by participating in Hacktoberfest. Happy coding!
Top comments (0)