Skip to content

Test2#2

Open
frontendpm wants to merge 2 commits intomainfrom
test2
Open

Test2#2
frontendpm wants to merge 2 commits intomainfrom
test2

Conversation

@frontendpm
Copy link
Copy Markdown
Owner

No description provided.

Comment thread main.js
str = name;
}

$('.social-link').on('click', function() {
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

not scoped to the component

Comment thread main.js
replaceNetworkName(socialNetworkName);
});

$('.button--ok').on('click', function() {
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

not scoped to the component

Comment thread main.js

$('.button--ok').on('click', function() {
//seems that redirection doesn't work on jsfiddle, but the function works correctly
window.location.href = 'http://' + socialNetworkName;
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

this is redirecting in same window and does not work in fiddle

Comment thread main.js
function replaceNetworkName(name) {
const regex = new RegExp(`${str}`, 'g')
const newhtml = $('.question').html().replace(regex, name);
$('.question').html(newhtml);
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

not cached, traversing dom

Comment thread main.scss

&:hover.button--cancel {
background-color: $cancel-button-color;
@include hover-transition(background-color);
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

include should be at top

Comment thread main.scss
&:hover {
@include hover-transition(transform);
box-shadow: 0 20px 30px rgba(0, 0, 0, 0.2);
transform: translateY(-0.5rem);
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

mixing rem and px

Comment thread main.scss
}

a {
@include button;
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

extend would be better

Comment thread main.scss
> div {
>div {
padding-bottom: 20px;
border-bottom: 1px solid #000;
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

lack of color variable

Comment thread main.scss
color: #000;

@include not-mobile {
font-size: $font-size-mobile + $font-size-mobile *0.2;
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

oO?

Comment thread main.scss
}

@mixin hover-transition($transition-name) {
transition: $transition-name 300ms ease-out 0.1s;
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

mixing units

Comment thread main.js
});

$('.button--ok').on('click', function() {
//seems that redirection doesn't work on jsfiddle, but the function works correctly
Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

There is an error message
Mixed Content: The page at 'https://jsfiddle.net/marwinn/1v3x58jo/6/' was loaded over HTTPS, but requested an insecure frame 'http://facebook.com/'. This request has been blocked; the content must be served over HTTPS.

You should try window.open instead for example

Comment thread main.js
function replaceNetworkName(name) {
const regex = new RegExp(`${str}`, 'g')
const newhtml = $('.question').html().replace(regex, name);
$('.question').html(newhtml);
Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

always looking for new '.question' instead of cached it

Comment thread main.js
let socialNetworkName = '';

function toggleDialog() {
$('.dialog').toggleClass('is-hidden');
Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

dialog should be cached

Comment thread main.scss
}
}

@mixin hover-transition($transition-name) {
Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

what for?

Comment thread main.scss
}

.social-links {
li {
Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

unnecessary nesting

Comment thread main.scss
&:hover {
@include hover-transition(transform);
box-shadow: 0 20px 30px rgba(0, 0, 0, 0.2);
transform: translateY(-0.5rem);
Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

now rems? why?

Comment thread main.scss
.container {
@include mobile {
display: flex;
flex-wrap: nowrap;
Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

it is default value

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.

2 participants