1014219 - [Messages] Consider using real <button>s instead of <a> links for the header icons

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::SMS, defect)

Description

[Julien Wajsberg [:julienw]]

The goal is to use real buttons for the actions.

We can argue that the "new message" button could be a link (as it should move to another panel with another hash in the end, with bug 1011089). But the request attachment and settings buttons can change.

Also, we should change the IDs for these buttons.

Comment 1

[Jai [:dolftax]]

May I take this?

Comment 2

[Steve Chung [:steveck]]

(In reply to Jaipradeesh [:jai] from comment #1)
> May I take this?

Welcome! Thanks for helping.

Comment 3

[Jai [:dolftax]]

Would you tell me how exactly should I proceed with this?

Comment 4

[Steve Chung [:steveck]]

If you have no experience in contributing gaia patch or even developing gaia codebase, please refer this link[1] first for more details. Don't hesitate to ask any question while in environment setup or developing.

If your question is about the solution of this issue, I think comment 0 already got sufficient information about implementation. Since we moved the attachment button to message input block and set as button already, you just need to the adjust settings button on header to button element with new precise ID name.

[1] https://developer.mozilla.org/en-US/Firefox_OS/Developing_Gaia

Comment 5

[Jai [:dolftax]]

Got my environment setup ready. Working on the bug.

Comment 6

[Anusha]

Hello,
I would like to work on this bug,
Please assign this bug to me,
Thank you.

Comment 7

[Julien Wajsberg [:julienw]]

Anusha, thanks for your interest.

As you see, someone already works on this issue :) I'm sure you can find other bugs on http://www.joshmatthews.net/bugsahoy/?b2g=1 ! 

Jaipradeesh, are you still working on this?

Comment 8

[Jai [:dolftax]]

Anusha,

You may proceed with this. Pretty busy with launch here. Apologies for the delay.

Comment 9

[Julien Wajsberg [:julienw]]

Oleg, is this bug fixed in bug 1074224?

Comment 10

[Oleg Zasypkin [:azasypkin]]

(In reply to Julien Wajsberg [:julienw] (PTO until 7th October, ask :steveck or :azasypkin for SMS stuff) from comment #9)
> Oleg, is this bug fixed in bug 1074224?

Nope, bug 1074224 removes only CSS selectors that used [role=button]. So this bug is still valid.

Comment 11

[Anusha]

Attachment: Bug 1014219 - [Messages] Consider using real <button>s instead of <a> links for the header icons

Comment 12

[Oleg Zasypkin [:azasypkin]]

Comment on attachment 8530205 [details] [review]
Bug 1014219 - [Messages] Consider using real <button>s instead of <a> links for the header icons

Thanks for the patch and sorry for the delay!

I've just left some comments at GitHub.

Please, set r? once you're ready!

Comment 13

[Anusha]

Attachment: Bug 1014219 - [Messages] Consider using real <button>s instead of <a> links for the header icons

Comment 14

[Anusha]

Oleg sorry for the delay had exams. Sent the patch.
Thanks.

Comment 15

[Oleg Zasypkin [:azasypkin]]

Comment on attachment 8532835 [details] [review]
Bug 1014219 - [Messages] Consider using real <button>s instead of <a> links for the header icons

(In reply to Anusha from comment #14)
> Oleg sorry for the delay had exams. Sent the patch.
> Thanks.

Hey Anusha, no worries, always take your time, as much as you need!

Everything looks good, but several tests are failed (Li - linter, f1,f2,f3 - pyhton integration tests). I've left more details at GitHub.

Regarding to PRs - you don't need to create a new PR every time. The usual flow is the following:

1. You create PR with initial changes (you already did it!);

2. After round of review, you're making changes in the same branch, create a separate commit with these new changes (I usually use commit message for such commit like "Review round №1: fixed this, added that...");

3. Push this new commit to the the same branch at GitHub, PR is automatically updated in this case. Moreover all tests are re-run with new changes.

When tests are restarted "try-server-hook" adds comment to the PR with the link to the test results (like this one [1]). Once all tests finished you can see if patch didn't break anything.

So please ask r? once you're ready!

[1] https://github.com/mozilla-b2g/gaia/pull/26652#issuecomment-65911274

Comment 16

[Anusha]

Attachment: Bug 1014219 - [Messages] Consider using real <button>s instead of <a> links for the header icons

Comment 17

[Oleg Zasypkin [:azasypkin]]

Comment on attachment 8534856 [details] [review]
Bug 1014219 - [Messages] Consider using real <button>s instead of <a> links for the header icons

Looks good now!

Now before we land the patch, please do the following:

1. Squash all your commits into one [1];

2. Add "r=azasypkin" at the end of commit message (will be like " Bug 1014219 - [Messages] Consider using real <button>s instead of <a> links for the header icons. r=azasypkin");

3. Push (with --force flag) your joined commit to the same branch;

4. Tests will be restarted again, and once they are green please add "checkin-needed" keyword to the bug ("Keywords:" field in bug description).

Thanks a lot for your work!

[1] https://ariejan.net/2011/07/05/git-squash-your-latests-commits-into-one/

Comment 18

[Anusha]

Comment on attachment 8534856 [details] [review]
Bug 1014219 - [Messages] Consider using real <button>s instead of <a> links for the header icons

https://github.com/mozilla-b2g/gaia/pull/26914

Comment 19

[Ryan VanderMeulen [:RyanVM]]

Master: https://github.com/mozilla-b2g/gaia/commit/3a8e75217255a281c92991994674a07ae6c48650