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

ui: Rename "All messages" to "Combined feed". #644

Merged
merged 1 commit into from
May 14, 2024
Merged

Conversation

Lalit3716
Copy link
Contributor

This closes: #634.

This is my first try to contribute here, so wanted to ask making this change didn't required any corresponding test changes?
Are we not testing like these buttons should be shown on home screen or something?

Screenshot:
Screenshot 2024-05-05 at 8 21 33 PM

Copy link
Collaborator

@chrisbobbe chrisbobbe left a comment

Choose a reason for hiding this comment

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

Thanks @Lalit3716!

I think it's OK not to have UI tests for the current home page, which is a temporary, prototype-level UI. (#311 describes how we plan to replace it.)

ui: Rename "All messages" to "Combined feed".

nit: this commit message should have a Fixes line, like other commits that fix issues in the tracker. I recommend looking at the project's history using Greg's "secret" for doing so efficiently, to get a sense of our commit-message style. Another small thing you'll notice is that we tend not to end the summary line with a period, so ui: Rename "All messages" to "Combined feed" is fine and a little bit shorter.

assets/l10n/app_en.arb Outdated Show resolved Hide resolved
@Lalit3716
Copy link
Contributor Author

Thanks @chrisbobbe for pointing out those nits and yeah it makes sense to not have tests that we will end up removing later 👍

@gnprice
Copy link
Member

gnprice commented May 14, 2024

Thanks @Lalit3716, and thanks @chrisbobbe for the initial review! Looks good — merging.

@gnprice
Copy link
Member

gnprice commented May 14, 2024

I've also just filed a followup issue #676 for completing this renaming in our codebase as well as the UI. @Lalit3716 if you'd like to pick up another issue, that would be a great next one to try.

@Lalit3716
Copy link
Contributor Author

Lalit3716 commented May 14, 2024

Thank you, I will pick this follow-up issue next, sounds straight forward to do :)

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.

Rename "All messages" to "Combined feed"
3 participants