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

[Blogging Prompts] Create Prompts List item #17707

Merged
merged 14 commits into from
Jan 4, 2023

Conversation

thomashorta
Copy link
Contributor

@thomashorta thomashorta commented Dec 29, 2022

Part of #17125

Add the UI component for the Prompts List item matching UI specs, and use it in the Prompts List screen.

Note: this is still using mocked data!

Video Demo:

prompts-list-items-demo.mp4

To test:

  1. Do the steps to enable the Prompts List feature flag found in the test section of [Blogging Prompts] Prompts list feature flag #17675
  2. Sign in (if not signed in already)
  3. Go to the home dashboard (My Site -> Home)
  4. Verify the Prompts card is shown
  5. Tap the 3-dot menu in the Prompts card
  6. Tap the "View more prompts" option
  7. Verify the Prompts List activity is shown
  8. Verify it shows a "Loading..." text for a while
  9. Verify the content is updated with a fake list of 11 items, matching the design spec

Regression Notes

  1. Potential unintended areas of impact
    N/A

  2. What I did to test those areas of impact (or what existing automated tests I relied on)
    N/A

  3. What automated tests I added (or what prevented me from doing so)
    UI changes only, which are not currently covered by tests.

PR submission checklist:

  • I have completed the Regression Notes.
  • I have considered adding accessibility improvements for my changes.
  • I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

@wpmobilebot
Copy link
Contributor

wpmobilebot commented Dec 29, 2022

Jetpack📲 You can test these changes on Jetpack by downloading jetpack-installable-build-pr17707-5004b5f.apk
💡 Scan this QR code with your Android phone to download and install the APK directly on it.
AppJetpack
Build FlavorJalapeno
Build TypeDebug
Commit5004b5f
Note: This installable build uses the JalapenoDebug build flavor, and does not support Google Login.

@wpmobilebot
Copy link
Contributor

wpmobilebot commented Dec 29, 2022

WordPress📲 You can test these changes on WordPress by downloading wordpress-installable-build-pr17707-5004b5f.apk
💡 Scan this QR code with your Android phone to download and install the APK directly on it.
AppWordPress
Build FlavorJalapeno
Build TypeDebug
Commit5004b5f
Note: This installable build uses the JalapenoDebug build flavor, and does not support Google Login.

Base automatically changed from issue/17125-prompts-list-mocked-data to trunk January 3, 2023 15:12
Copy link
Contributor

@RenanLukas RenanLukas 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 PR, @thomashorta . It's working as expected 🎉

There's just one thing I wanted to ask about the app language. Here I had Portuguese (Brazil) selected in the App Settings but the formatted date remains in English. Maybe we should be using something else instead of LocaleManager#getSafeLocale? 🤔

image

image

It contains colors that are part of Automattic's Design System (Color Studio) as
well as colors commonly used in the app (such as DarkGray).
Move the date formatting that was happening in the UI / View layer of the Prompt
List Item to a Mapper and do it in the presentation (VM) layer. This makes the
UseCase more reusable by using a Domain model (from FluxC) and removes the date
formatting objects from Compose UI.
@thomashorta
Copy link
Contributor Author

Thanks for the PR, @thomashorta . It's working as expected 🎉

There's just one thing I wanted to ask about the app language. Here I had Portuguese (Brazil) selected in the App Settings but the formatted date remains in English. Maybe we should be using something else instead of LocaleManager#getSafeLocale? 🤔

With the latest changes, using LocaleManagerWrapper this seems to be working correctly. Just a note that if the screen is opened already when the language is changed, it might need to be closed and opened again.

this fixes the issue of this screen not using the right Locale when the language
settings are changed INSIDE the app.
Copy link
Contributor

@RenanLukas RenanLukas 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 changes, @thomashorta

LGTM :shipit:

@thomashorta
Copy link
Contributor Author

With the latest changes, using LocaleManagerWrapper this seems to be working correctly. Just a note that if the screen is opened already when the language is changed, it might need to be closed and opened again.

So, I was wrong about this one, I was under the impression @RenanLukas mentioned the Android System Language Settings, which were working in my last comment. But changing the language inside the Jetpack App settings was still showing the wrong date formatting.

Just have in mind that other strings in that screen are currently hardcoded or just added to strings.xml, that's why they are not translated yet.

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

3 participants