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

Remove Opera from Dashboard #586

Merged
merged 7 commits into from
Feb 27, 2019
Merged

Remove Opera from Dashboard #586

merged 7 commits into from
Feb 27, 2019

Conversation

jpmedley
Copy link
Contributor

@jpmedley jpmedley commented Feb 5, 2019

Eric,

A while back we discussed removing Opera from Chrome Status. If I remember correctly, we agreed that it should go.

I've started by commenting what I think should be removed. Let me know if I've made any mistakes or omissions. Then I'll remove it for real.

Thanks,
Joe

@jpmedley jpmedley requested a review from ebidel February 5, 2019 22:05
@@ -165,7 +165,7 @@ <h3>Chromium status</h3>
<span>[[feature.browsers.chrome.webview]]</span>
</span>
</template>
<template is="dom-if" if="[[!feature.browsers.chrome.origintrial]]">
<!-- <template is="dom-if" if="[[!feature.browsers.chrome.origintrial]]">
Copy link
Contributor

Choose a reason for hiding this comment

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

Think you still want to keep this outer conditional. Just comment out the inner opera bits.

Copy link
Contributor Author

@jpmedley jpmedley Feb 13, 2019

Choose a reason for hiding this comment

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

The inner Opera is the only thing inside this particular tag which is why its included. The closing template is on line 192.

Copy link
Contributor

Choose a reason for hiding this comment

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

I see now. Wasn't reading the diff correctly.

@ebidel
Copy link
Contributor

ebidel commented Feb 11, 2019

@jpmedley
Copy link
Contributor Author

PTAL

Should be change the switch to an if in feature_form.js?

@ebidel
Copy link
Contributor

ebidel commented Feb 13, 2019

You can leave it a switch. Can you remove the actual code bits?

@jpmedley
Copy link
Contributor Author

can you look at this line. There should be a closing {% endif %} for it, but I'm having trouble seeing which one it is.

@jstenback
Copy link
Collaborator

Joe, I'm not clear on exactly which line you're referring to in that diff (the link doesn't point to a specific line), but AFAICT that diff as a whole is well balanced in terms of if and endif statements.

@jpmedley
Copy link
Contributor Author

The link points to 130 when I follow it.

My changes to that file have 4 'if's and 3 'endif's.

@jstenback
Copy link
Collaborator

Ah, I see, apologies for not noticing that. The issue, FAICT, is that you're leaving an {% if ... %} block in the file inside of an HTML comment. That still likely gets evaluated and leads to a mismatch (see https://stackoverflow.com/questions/719915/how-to-put-comments-in-django-templates). If you'd remove that whole line instead, or use {% #... } for that line, things ought to work. The matching endif is here in case that helps.

@jpmedley
Copy link
Contributor Author

I'm not leaving it inside a comment. I commented the code initially because we don't have a staging server for this and I wanted Eric to sanity check the work before I proceeded. (Uncommenting is quicker than restoring deleted lines.)

I see where I messed up now. I had already included the replacement line (which is where the extra 'if' came from). I had forgotten why that other line was there.

@jpmedley
Copy link
Contributor Author

@ebidel I think I'm ready for final review.

@jpmedley jpmedley changed the title [NOT FOR MERGING] Start removing Opera. Remove Opera from Dashboard Feb 21, 2019
@jpmedley
Copy link
Contributor Author

@ebidel Is that Lighthouse error because of something I touched? There's no link that would let me get more information.

(Is this something I'll be able to investigate when I get this running locally?)

@jpmedley
Copy link
Contributor Author

When this is merged, I should do the deployment to see if anything goes wrong.

@jpmedley
Copy link
Contributor Author

I missed something. I just tested locally. Opera is gone from the edit forms, but still appears in status entries.

Do we want to continue displaying the data we have?
@jstenback @PaulKinlan

@ebidel
Copy link
Contributor

ebidel commented Feb 21, 2019

Ignore the Lighthouse bot.

If you're running locally and have made changes to the polymer elements (you have), you'll need to run gulp vulcanize or set VULCANIZE = False in settings.py to see your updates. I recommend running gulp so you don't accidentally deploy unminified web components.

https://github.com/GoogleChrome/chromium-dashboard#debugging--settings

@jpmedley
Copy link
Contributor Author

I ran gulp vulcanize. I still see Opera on my Status entries.

package.json Outdated Show resolved Hide resolved
static/js/admin/feature_form.js Outdated Show resolved Hide resolved
@ebidel ebidel merged commit c705e15 into master Feb 27, 2019
@ebidel ebidel deleted the rmopera branch February 27, 2019 20:47
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.

3 participants