Skip to content

Fix IE and minor tweaks#2392

Merged
3 commits merged into
nodejs:masterfrom
XhmikosR:master-xmr-ie
Aug 21, 2019
Merged

Fix IE and minor tweaks#2392
3 commits merged into
nodejs:masterfrom
XhmikosR:master-xmr-ie

Conversation

@XhmikosR
Copy link
Copy Markdown
Contributor

@XhmikosR XhmikosR commented Aug 12, 2019

Before:

1-before

After:

2-after

@XhmikosR XhmikosR marked this pull request as ready for review August 12, 2019 16:40
@XhmikosR
Copy link
Copy Markdown
Contributor Author

XhmikosR commented Aug 12, 2019

BTW I can always rebase and generally make any needed changes. I just happened to notice the IE issues and maybe the last patch could be split to a separate PR.

@XhmikosR XhmikosR mentioned this pull request Aug 15, 2019
Merged
@XhmikosR
Add width and height in lang selection SVG.
9467e62 
Otherwise this didn't show properly on IE.
@XhmikosR
header.hbs: use ES5.
a0080d3
@XhmikosR
Move border: none in _base.styl.
eda02bf 
This way none of the images will have the border.
@ghost ghost self-requested a review August 20, 2019 05:22
ghost
ghost reviewed Aug 20, 2019
View reviewed changes
Copy link
Copy Markdown

@ghost ghost left a comment

Choose a reason for hiding this comment

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

Any special changes？I don't see anything perticular……？

@ghost ghost requested review from alexandrtovmach and diagramatics August 20, 2019 05:23
alexandrtovmach
alexandrtovmach reviewed Aug 20, 2019
View reviewed changes
Copy link
Copy Markdown
Contributor

@alexandrtovmach alexandrtovmach left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Looks good for me, but I haven't sure about these changes. I can't believe that is some developer using old version of IE. In the same time you refactored JS code to ES5 style, that's not very readable (especially Array.prototype stuff).

@alexandrtovmach
Copy link
Copy Markdown
Contributor

alexandrtovmach commented Aug 20, 2019 •
edited
Loading

Any special changes？I don't see anything perticular……？

@MaledongGit
Take a look on language switcher, it wasn't show before in IE

@XhmikosR
Copy link
Copy Markdown
Contributor Author

XhmikosR commented Aug 20, 2019

Looks good for me, but I haven't sure about these changes. I can't believe that is some developer using ol version of IE. In the same time you refactored JS code to ES5 style, that's not very readable (especially Array.prototype stuff).

Well, this is the only place in frontend that ES6 was used. And that's just one line, we can all live with it or with [].slice.call :)

@ghost
Copy link
Copy Markdown

ghost commented Aug 21, 2019

Any special changes？I don't see anything perticular……？

@MaledongGit
Take a look on language switcher, it wasn't show before in IE

Thanks for telling me, it's right.
I've no doubt about this change, but I'll see others' opinions about this change. I'm OK with it.

@ghost ghost self-requested a review August 21, 2019 00:19
@silverwind
Copy link
Copy Markdown
Contributor

silverwind commented Aug 21, 2019

That var change is kind of pointless thought. IE11 supports const and supporting anything older is also pointless.

@XhmikosR
Copy link
Copy Markdown
Contributor Author

XhmikosR commented Aug 21, 2019

@silverwind: it's the only place ES6 is used. And IE doesn't support arrow functions. I went with consistency here, which is better in the long-term IMO.

@silverwind
Copy link
Copy Markdown
Contributor

silverwind commented Aug 21, 2019

Ok, whatever. Would prefer we stop supporting IE11 soon and compile to ES6.

@XhmikosR
Copy link
Copy Markdown
Contributor Author

XhmikosR commented Aug 21, 2019

Fine by me but it should be done properly is all I'm saying. And this was easier for me at the time :P

@ghost ghost merged commit 7bbb782 into nodejs:master Aug 21, 2019
@XhmikosR XhmikosR deleted the master-xmr-ie branch August 21, 2019 08:33
@ghost ghost mentioned this pull request Aug 23, 2019
Merged
ghost pushed a commit that referenced this pull request Aug 24, 2019
fix: Make the tooltip visible in IE10+ (#2452)
689783f 
Notice:

This 'rect' is invisible as a container for the language symbol
to show the tooltip in both IE and Chrome, FF,ect.

For reasons of accessibility, user agents should always make the content
of the 'title' child element to the root svg element available to users.
However, this is typically done through other means than the tooltips
used for nested SVG and graphics elements, e.g., by displaying in a browser
tab.

See more at: https://svgwg.org/svg2-draft/struct.html#TitleElement

Issues and Subjects related to this are：
1、#2294.
2、#2403.
3、#2392.
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@diagramatics diagramatics Awaiting requested review from diagramatics

3 more reviewers

@alexandrtovmach alexandrtovmach alexandrtovmach left review comments

@silverwind silverwind silverwind approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@XhmikosR @alexandrtovmach @silverwind