Skip to content

Feature/david/snackbar updates #1056

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 10 commits into from
Feb 12, 2021
Merged

Feature/david/snackbar updates #1056

merged 10 commits into from
Feb 12, 2021

Conversation

@malmstein
Copy link
Contributor

@malmstein malmstein commented Jan 8, 2021 •
edited by cmonfortep
Loading

https://app.asana.com/0/414730916066338/1199702927327729/f
Adds missing tests for #951

Screenshots:

Fireproof Bookmarks
device-2021-02-05-135947 device-2021-02-05-135931

anoopaneesh and others added 7 commits October 1, 2020 17:32
@anoopaneesh
added snackbar
4734cf8
@anoopaneesh
Merge remote-tracking branch 'upstream/develop' into develop
3a1b306
@anoopaneesh
corrected styling issues
95177cf
@anoopaneesh
removed string resources and added new resources in string-untranslated
edf6eb1
@anoopaneesh
removed string resources and added new resources in string-untranslated
35d4795
@malmstein
Merge branch 'develop' of https://github.com/anoopaneesh/Android into…
b6078f0 
… anoopaneesh-develop

# Conflicts:
#	app/src/main/res/values-bg/strings.xml
#	app/src/main/res/values-cs/strings.xml
#	app/src/main/res/values-da/strings.xml
#	app/src/main/res/values-de/strings.xml
#	app/src/main/res/values-el/strings.xml
#	app/src/main/res/values-es/strings.xml
#	app/src/main/res/values-et/strings.xml
#	app/src/main/res/values-fi/strings.xml
#	app/src/main/res/values-fr/strings.xml
#	app/src/main/res/values-hr/strings.xml
#	app/src/main/res/values-hu/strings.xml
#	app/src/main/res/values-it/strings.xml
#	app/src/main/res/values-lt/strings.xml
#	app/src/main/res/values-lv/strings.xml
#	app/src/main/res/values-nb/strings.xml
#	app/src/main/res/values-nl/strings.xml
#	app/src/main/res/values-pl/strings.xml
#	app/src/main/res/values-pt/strings.xml
#	app/src/main/res/values-ro/strings.xml
#	app/src/main/res/values-ru/strings.xml
#	app/src/main/res/values-sk/strings.xml
#	app/src/main/res/values-sl/strings.xml
#	app/src/main/res/values-sv/strings.xml
#	app/src/main/res/values-tr/strings.xml
#	app/src/main/res/values/string-untranslated.xml
#	app/src/main/res/values/strings.xml
@malmstein
added missing tests and updated strings
b943f3a
@malmstein malmstein mentioned this pull request Jan 8, 2021
Closed
@malmstein malmstein self-assigned this Jan 8, 2021
@malmstein malmstein requested a review from cmonfortep January 8, 2021 14:13
cmonfortep
cmonfortep reviewed Jan 14, 2021
View reviewed changes
Copy link
Contributor

@cmonfortep cmonfortep left a comment

Choose a reason for hiding this comment

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

Everything looks great. Just a few comments from my side about the undo functionality:

  • the position of the element in the list changes
  • the favicon is lost
  • we should validate the new copy with the team ( in #945 the proposed copy was different)

What do you think? maybe the latter is more important. As for the rest, I will leave it up to you, no strong opinions from my side since this is just a nice to have functionality.

@malmstein
Copy link
Contributor Author

malmstein commented Jan 29, 2021

Thanks @cmonfortep !

On your comments:

  • Position is reordered, I don't see this is a problem
  • Favicon is not lost in any of my tests, let's sync and see what are we doing different
  • Agree, I've opened a task to get this reviewed

@malmstein
Copy link
Contributor Author

malmstein commented Feb 5, 2021

@cmonfortep PR updated with the Copy reviewed

cmonfortep
cmonfortep approved these changes Feb 12, 2021
View reviewed changes
Copy link
Contributor

@cmonfortep cmonfortep left a comment

Choose a reason for hiding this comment

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

LGTM, sorry for the late review. This looks great. You are right, favicon does not disappear...maybe we fixed something in between reviews.

Anyways, ship it! (but check that versions.properties file, not sure why it's there)

versions.properties
@@ -0,0 +1,41 @@
## suppress inspection "SpellCheckingInspection" for whole file
Copy link
Contributor

@cmonfortep cmonfortep Feb 12, 2021

Choose a reason for hiding this comment

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

👀

Copy link
Contributor Author

@malmstein malmstein Feb 12, 2021

Choose a reason for hiding this comment

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

This is from refreshVersions plugin, part of #1090

@malmstein
Copy link
Contributor Author

malmstein commented Feb 12, 2021

Thanks for the review @cmonfortep !

@malmstein malmstein merged commit 49a3693 into develop Feb 12, 2021
@malmstein malmstein deleted the feature/david/snackbar-updates branch February 12, 2021 16:28
@cmonfortep cmonfortep mentioned this pull request Apr 8, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@cmonfortep cmonfortep cmonfortep approved these changes

Assignees

@malmstein malmstein

@cmonfortep cmonfortep

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@malmstein @cmonfortep @anoopaneesh