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

2143 - display bookmarked moments in care plan #2154

Merged
merged 8 commits into from
Oct 11, 2022

Conversation

yasirazgar
Copy link
Contributor

Description

Enable ability to bookmark Moments and display them on the Care Plan page

More Details

Corresponding Issue

#2143

Screenshots

  1. Care Plan Page

Screenshot 2022-10-07 at 10 44 20 AM

  1. Bookmark moments tool tip

Screenshot 2022-10-07 at 10 44 46 AM


Reviewing this pull request? Check out our Code Review Practices guide if you haven't already!

Copy link
Member

@julianguyen julianguyen left a comment

Choose a reason for hiding this comment

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

Thanks so much for taking this on! Hope you're well :)

Have some feedback and after that, we should be good to go!

@@ -0,0 +1,18 @@
/* eslint no-console:0 */
Copy link
Member

Choose a reason for hiding this comment

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

Let's delete this file since it's not necessary!

@@ -0,0 +1,5 @@
process.env.NODE_ENV = process.env.NODE_ENV || 'development'
Copy link
Member

Choose a reason for hiding this comment

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

Let's also remove this file!

@@ -0,0 +1,3 @@
const { environment } = require('@rails/webpacker')
Copy link
Member

Choose a reason for hiding this comment

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

Let's also remove this file!

@@ -0,0 +1,5 @@
process.env.NODE_ENV = process.env.NODE_ENV || 'production'
Copy link
Member

Choose a reason for hiding this comment

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

Let's also remove this file!

@@ -0,0 +1,5 @@
process.env.NODE_ENV = process.env.NODE_ENV || 'development'
Copy link
Member

Choose a reason for hiding this comment

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

Let's also remove this file!

@@ -19,6 +19,17 @@
expect(response.body).to include(strategy.name)
end
end

context 'when there are bookmarked moments' do
let!(:strategy_bm) { create(:strategy, user: user, bookmarked: true) }
Copy link
Member

Choose a reason for hiding this comment

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

This test should replace the strategy variables with moment ones!

@yasirazgar
Copy link
Contributor Author

yasirazgar commented Oct 10, 2022

@julianguyen I have addressed your feedbacks, apologies added some generated files by mistake, removed them now.

Copy link
Member

@julianguyen julianguyen left a comment

Choose a reason for hiding this comment

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

Thanks for the updates! 🎉 One more fix and we should be good to merge!

@@ -87,7 +87,8 @@ def moment_bookmarked
'bookmarked', 'switch', 'moments.form.bookmarked_question'
).merge(
value: true, uncheckedValue: false,
checked: @moment.bookmarked, dark: true
checked: @moment.bookmarked, dark: true,
Copy link
Member

Choose a reason for hiding this comment

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

checked: @moment.bookmarked should be checked: params[:bookmarked] ? true : @moment.bookmarked so that when you create a Moment from the Care Plan page, it will automatically be bookmarked.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah gotcha, updated now.

Copy link
Member

@julianguyen julianguyen left a comment

Choose a reason for hiding this comment

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

Awesome work, thanks so much for taking this on 🎉

@julianguyen julianguyen merged commit a12a327 into ifmeorg:main Oct 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants