[Bug 826085 ][B2G] [SMS] Contacts : SMS message deleted when choosing contact for sea... #7394

Merged
merged 1 commit into from Jan 9, 2013

Conversation

Projects
None yet
3 participants
Member

borjasalguero commented Jan 8, 2013

...rch or contact list

Member

borjasalguero commented Jan 8, 2013

There are some small changes coming, I will ping u again when ready!

Member

borjasalguero commented Jan 8, 2013

@steveck-chung Code ready for reviewing! All @aymanmaat suggestions added.r?
@stasm r?

Member

borjasalguero commented Jan 8, 2013

I will squash after the review!

@steveck-chung steveck-chung and 1 other commented on an outdated diff Jan 9, 2013

apps/sms/locales/sms.en-US.properties
@@ -44,6 +44,7 @@ no-results = No results returned
# Modal Dialogs
resend-confirmation = The message could not be sent. Try again?
+resend-confirmation = The message could not be sent. Try again?
@steveck-chung

steveck-chung Jan 9, 2013

Contributor

Why we need a duplicate string here...? Is this new string for discard sms (discard-sms)?

@borjasalguero

borjasalguero Jan 9, 2013

Member

Arrr, my fault! I dont know why it's not in the commit :S. Updating!

@steveck-chung steveck-chung and 1 other commented on an outdated diff Jan 9, 2013

apps/sms/js/sms.js
@@ -106,7 +106,18 @@ var MessageManager = {
} else if (threadMessages.classList.contains('new')) {
MessageManager.slide(function() {
threadMessages.classList.remove('new');
@steveck-chung

steveck-chung Jan 9, 2013

Contributor

We can move the class remove logic after user responding to reduce the unnecessary animation. If user select cancel, we do nothing, otherwise we will remove 'new' class and clean the field.
Another question: Why we only consider in 'new' thread? If the thread view already has the number as hash, can we just discard message directly?

@borjasalguero

borjasalguero Jan 9, 2013

Member

+1. Completely agree.

@steveck-chung steveck-chung and 1 other commented on an outdated diff Jan 9, 2013

apps/sms/js/sms.js
@@ -1170,12 +1181,24 @@ var ThreadUI = {
}
},
- cleanFields: function thui_cleanFields() {
- this.sendButton.disabled = true;
- this.sendButton.dataset.counter = '';
- this.contactInput.value = '';
- this.input.value = '';
- this.updateInputHeight();
+ cleanFields: function thui_cleanFields(sending) {
@steveck-chung

steveck-chung Jan 9, 2013

Contributor

It might be better to use the tern like forceClean. In certain situation like send or back to list view, you can clean the fields directly without other consideration.

@borjasalguero

borjasalguero Jan 9, 2013

Member

+1. I've modified the logic in order to force the cleaning if needed.

@steveck-chung steveck-chung and 1 other commented on an outdated diff Jan 9, 2013

apps/sms/js/sms.js
- this.input.value = '';
- this.updateInputHeight();
+ cleanFields: function thui_cleanFields(sending) {
+ var self = this;
+ var clean = function clean() {
+ self.input.value = '';
+ self.sendButton.disabled = true;
+ self.sendButton.dataset.counter = '';
+ self.contactInput.value = '';
+ self.updateInputHeight();
+ };
+ if (window.location.hash == this.previousHash ||
+ this.previousHash == '#new') {
+ if (sending) {
+ clean();
+ }
@steveck-chung

steveck-chung Jan 9, 2013

Contributor

You should update the button status(enableSend) if you don't force to clean the fields(or you will see the button still disabled after add the receiver from contact)

@borjasalguero

borjasalguero Jan 9, 2013

Member

With new paradigm, when you are requested in 'thread-view'/'new-message-view', I think it's working.

@steveck-chung

steveck-chung Jan 9, 2013

Contributor

Hmm.... The button still disabled. Have you tried the scenario(Add New -> type text content -> pick a contact)? The message will exist but the button is disabled even you are in the thread-view.

Member

borjasalguero commented Jan 9, 2013

Summary. I've been talking with @aymanmaat and we agree with your comment #7394 (comment) , so now the user will be asked to discard the message if the input it's not empty in both cases (new message / new message inside a thread). All your suggestions are added, r?

Member

borjasalguero commented Jan 9, 2013

All commits squashed and I've updated the commit name with the reviewer.

@fabi1cazenave fabi1cazenave and 1 other commented on an outdated diff Jan 9, 2013

apps/sms/locales/sms.en-US.properties
@@ -43,6 +43,7 @@ carrier-unknown = Carrier unknown
no-results = No results returned
# Modal Dialogs
+discard-sms = Are you sure you want to discard this message?
@fabi1cazenave

fabi1cazenave Jan 9, 2013

Contributor

nit, would you align the = sign with the rest please?

@borjasalguero

borjasalguero Jan 9, 2013

Member

Done. Thanks for the review @fabi1cazenave !

Contributor

steveck-chung commented Jan 9, 2013

Except for the button status, I think the rest of part is ok for me. You can ping me if you still unable to reproduce the issue.

Member

borjasalguero commented Jan 9, 2013

@steveck-chung Button status fixed! Sorry for the delay I was reviewing other patch!

borjasalguero merged commit 92ed2a2 into mozilla-b2g:master Jan 9, 2013

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment