Closed Bug 903611 Opened 12 years ago Closed 8 years ago

Change link button to use <button> or <span>

Categories

(Webmaker Graveyard :: Popcorn Maker, defect)

Product:
Webmaker Graveyard
Component:
Popcorn Maker
Platform:
x86
macOS
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED INCOMPLETE

People

(Reporter: alicoding, Assigned: negomi, Mentored)

Details

(Whiteboard: [good first bug])

Attachments

(1 file, 1 obsolete file)

https://github.com/mozilla/popcorn.webmaker.org/pull/600
56 bytes, text/x-github-pull-request
mjschranz
: review+
Details | Review
We have a link in Popcorn maker to close or hide the message. We want to change from using <a href> to <button> or <span> @k88hudson do you have any suggestion?
Flags: needinfo?(kate)
Flags: needinfo?(kate)
Which link is this?
The UA_WARNING, and the rest of them.
Hi, If no one is working on this then please assign this to me. Thanks, Zak IRC: Vader
Also which link's are we talking about. Is it all links on all popcorn maker pages.
file -> locale/en_US/popcorn.webmaker.org.json:24 output: "Click to remove warning": "Click <a href=\"#\" class=\"close-button\">here</a> to remove this warning.", Only one of them left to do now
Assignee: nobody → zak.hassan1010
Whiteboard: [mentor=thecount][mentor=mjschranz][mentor=aali]
@aali: I found out that the following files have <a href="#"></a> however I will fix the mistake you highlighted in your comment. > locale/bn_BD/popcorn.webmaker.org.json > locale/bn_IN/popcorn.webmaker.org.json > locale/en_CA/popcorn.webmaker.org.json > locale/en_US/popcorn.webmaker.org.json > locale/es/popcorn.webmaker.org.json > locale/fr/popcorn.webmaker.org.json > locale/pt/popcorn.webmaker.org.json > locale/ru/popcorn.webmaker.org.json > locale/th_TH/popcorn.webmaker.org.json > public/external/stamen/tile.stamen-1.2.0.js > public/src/util/social-media.js > public/templates/assets/plugins/wikipedia/popcorn.wikipedia.js > public/test/manual-tests/index.html > public/test/manual-tests/test-add.html > public/test/manual-tests/test-export.html > public/test/manual-tests/test-save.html > public/test/manual-tests/test-share.html > public/test/manual-tests/test-sharevimeo.html > public/test/manual-tests/test-shareyoutube.html > public/test/manual-tests/test-vimeo.html > public/test/manual-tests/test-youtube.html > public/test/qunit/qunit.js > views/embed-shell.html > views/embed.html > views/layouts/header.html
@Zak, for popcorn.webmaker.org.json please only fix in locale/en_US/ only and other files I have gone through them and they are not exactly the same from what in the ticket asking to change, so I wouldn't change them at this time.
@aali: I rolled back my changes and you should see only the file requested originally in this pull request
Fix complete
Attachment #830604 - Flags: review?(ali)
Whiteboard: [mentor=thecount][mentor=mjschranz][mentor=aali] → [mentor=thecount][mentor=mjschranz][mentor=aali][good first bug]
Currently the patch you have only change the element but no style on the button at all. https://dl.dropboxusercontent.com/spa/n5v3bx9nnjkdpzf/kojsyy9m.png I think k88hudson can suggest something.
Flags: needinfo?(kate)
IMO it would be better to use a more conventional sort of close button, i.e. this: https://dl.dropboxusercontent.com/spa/udzcfdb4yizzlua/ut6otgrq.png there is an icon class called .icon-remove that you can use for the x
Flags: needinfo?(kate)
Comment on attachment 830604 [details] [review] https://github.com/mozilla/popcorn.webmaker.org/pull/308 r- for now. You may want to look at this example: https://github.com/mozilla/login.webmaker.org/blob/master/app/http/public/css/account.less#L259-L272 I have done the same with the login.webmaker.org/account when they're trying to remove their account.
Attachment #830604 - Flags: review?(ali) → review-
Comment on attachment 830604 [details] [review] https://github.com/mozilla/popcorn.webmaker.org/pull/308 Ready for landing. I've rebased the code and fixed the code please review.
Attachment #830604 - Flags: review- → review?(ali)
Comment on attachment 830604 [details] [review] https://github.com/mozilla/popcorn.webmaker.org/pull/308 I'm going to have to r- this one for now until the update as we discussed at CDOT. If you can't complete this one just let me know :)
Attachment #830604 - Flags: review?(ali) → review-
Okay I will be working on this in tomorrow morning morning.
Status: NEW → ASSIGNED
Do you think you can finish this bug? Or should this be assign to someone else?
Assignee: zak.hassan1010 → nobody
Flags: needinfo?(zak.hassan1010)
Hi Ali, You can take this.
Flags: needinfo?(zak.hassan1010)
Mentor: scott schranz.m ali
Mentor: ali, schranz.m, scott
Whiteboard: [mentor=thecount][mentor=mjschranz][mentor=aali][good first bug] → [good first bug]
Mentor: ali, schranz.m, scott
Hi, could I be assigned to this bug please? Thanks!
Sure thing Imogen! Let us know if you need any additional info/help.
Assignee: nobody → negomi.code
Attachment #830604 - Attachment is obsolete: true
Comment on attachment 8586976 [details] [review] https://github.com/mozilla/popcorn.webmaker.org/pull/600 This is really great and works well but after seeing the completed solution I have one thing I would like to add. Seeing that now the key "Click to remove warning" contains only HTML that is to be rendered could we just remove that key and have the HTML for the button be static in the layout file (warn.html)? Thanks!
Attachment #8586976 - Flags: review?(schranz.m) → review-
Comment on attachment 8586976 [details] [review] https://github.com/mozilla/popcorn.webmaker.org/pull/600 Definitely, having it static makes a lot more sense. I've updated the PR.
Attachment #8586976 - Flags: review- → review?(schranz.m)
Comment on attachment 8586976 [details] [review] https://github.com/mozilla/popcorn.webmaker.org/pull/600 R+ :) Do you know how to squash your commits? If you do please just use your first commit as the message. If not we can do it on the way in for the merge. Thanks!
Attachment #8586976 - Flags: review?(schranz.m) → review+
Flags: needinfo?(negomi.code)
Yay! Scott can you merge this? Thanks
Flags: needinfo?(scott)
Flags: needinfo?(scott)
Popcorn Maker is no longer under active development. https://learning.mozilla.org/blog/product-update-for-appmaker-and-popcorn-maker
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → INCOMPLETE
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: