Skip to content
This repository was archived by the owner on Feb 22, 2024. It is now read-only.

Conversation

@habib-deriv
Copy link
Contributor

Q                       A
Fixed Issues? Fixes #1, Fixes #2
Patch: Bug Fix?
Major: Breaking Change?
Minor: New Feature?
Tests Added + Pass? Yes
Does not reduce coverage? Yes
Any Dependency Changes?

mitra-deriv and others added 30 commits May 12, 2022 18:03
…ction

Mitra/Add exclusively on deriv section
Component = <BotLanding />;
dynamicVar = 'bot-landing';
}
console.log(Component, dynamicVar);
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to keep the console here?

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks @prince-deriv, this is removed 🙏

@habib-deriv habib-deriv marked this pull request as ready for review July 25, 2022 05:20
let Component, dynVar;
if (window.location.pathname === '/movetoderv.html') {
Component = <BinaryLanding />;
dynVar = 'movetoderiv';
Copy link
Contributor

Choose a reason for hiding this comment

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

so where are we using dynVar?

render(<BotLanding />, document.getElementById('bot-landing'));
setStorage('setDueDateForBanner', expirationDate());
let Component, dynamicVar;
if (window.location.pathname === '/movetoderiv.html') {
Copy link
Contributor

Choose a reason for hiding this comment

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

We are repeating the same thing in renderReactComponents .. can we use 1 component?

const renderElements = () => {
// eslint-disable-next-line one-var, no-unused-vars
let Component, dynamicVar;
if (window.location.pathname === '/movetoderiv.html') {
Copy link
Contributor

Choose a reason for hiding this comment

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

again here

autoplaySpeed : 3000,
responsive : [
{
breakpoint: 1024,
Copy link
Contributor

Choose a reason for hiding this comment

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

can we use the breakpoint variables instead of 1024?

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks @sara-fs, This is actually the carousel setting itself and we are using it once only 🙏

},
},
{
breakpoint: 700,
Copy link
Contributor

Choose a reason for hiding this comment

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

like here

<img src="image/binary.png" />
</a>
</div>
<a href="https://oauth.deriv.com/oauth2/authorize?app_id=16929&l=en&brand=deriv" className="navigation-to-deriv" rel="noopener noreferrer">
Copy link
Contributor

Choose a reason for hiding this comment

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

appid should be const here?

</a>
</div>
<a href="https://oauth.deriv.com/oauth2/authorize?app_id=16929&l=en&brand=deriv" className="navigation-to-deriv" rel="noopener noreferrer">
<button className='btn-group r-btn'>
Copy link
Contributor

Choose a reason for hiding this comment

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

it was better and moe readable if we used complete word for variable names like right-btn

{translate('Just log in using your Binary.com credentials. No sign-up needed.')}
</h2>
<div className="btn-group">
<a href="https://oauth.deriv.com/oauth2/authorize?app_id=16929&l=en&brand=deriv" rel="noopener noreferrer">
Copy link
Contributor

Choose a reason for hiding this comment

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

same here


h1 {
text-align: center;
margin-top: 80px;
Copy link
Contributor

Choose a reason for hiding this comment

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

rem or px?

}
}

.carousel_section {
Copy link
Contributor

Choose a reason for hiding this comment

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

the name should be carousel-section
_ underline is being used to mention the element and -- for mentioning the modifier so to avoid confusion we usually dont use _ for the root name
http://getbem.com/naming/

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks dear @sara-fs, we will create a new PR for these changes later 🙏

.carousel_section {

&__desktop {
@media (max-width: 768px) {
Copy link
Contributor

Choose a reason for hiding this comment

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

no const variables for break points?


}

&__slide_img {
Copy link
Contributor

Choose a reason for hiding this comment

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

__slide-img
and the rest ...


// .language_list {
// padding-top: 26px !important;
// display: flex !important;
Copy link
Contributor

Choose a reason for hiding this comment

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

can we remove this ?

@sara-fs sara-fs merged commit 687e1f0 into master Jul 26, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants