BrandLogo Component #46

samad-yar-khan · 2022-01-23T15:02:16Z

I have read the Contributing Guide - https://github.com/RocketChat/Rocket.Chat/blob/develop/.github/CONTRIBUTING.md#contributing-to-rocketchat doc
I have signed the CLA - https://cla-assistant.io/RocketChat/Rocket.Chat
Lint and unit tests pass locally with my changes
I have added tests that prove my fix is effective or that my feature works (if applicable)
I have added necessary documentation (if applicable)
Any dependent changes have been merged and published in downstream modules

Proposed changes (including videos or screenshots)

I have added the Rocket.Chat logo in the NavBar in place of the text - Rocket.Chat Community

Issue(s)

This current master branch did not have the image tag at all but this should fix the below issue aswell.
Fixes : #43

Steps to test or reproduce

Mentioned in the Issue

Before :

After :

CLAassistant · 2022-01-23T15:02:20Z

All committers have signed the CLA.

Sing-Li

remove gitpod.yml

It should not be ommitted

Sing-Li

Instead of hacking this into the Navbar, how about creating a totally independent navbarlogo React component that is optionally composable into Navbar, in any position, and for ANY logo and branding.

Be sure to take advantage of NextJS packing and lazy loading when crafting component.

Sing-Li

create new component

samad-yar-khan · 2022-01-23T15:29:17Z

Instead of hacking this into the Navbar, how about creating a totally independent navbarlogo React component that is optionally composable into Navbar, in any position, and for ANY logo and branding.

Be sure to take advantage of NextJS packing and lazy loading when crafting component.

Great Idea, I will create a new component .

samad-yar-khan · 2022-01-23T17:01:01Z

@Sing-Li Can you look into this ? I read the docs regarding NextJS packing and lazy loading , I can incorporate the feature to only load the logo image when its viewed to increase efficiency but I don't think this would be much useful for this particular component as the navbar will always be loaded first and so will the brand logo. We can surely utilize lazy loading with some of the larger graphics components which are shown as we scroll further down. Let me know if I am thinking in the right direction :)

RonLek · 2022-01-23T18:34:47Z

app/components/menubar.js

@@ -1,16 +1,20 @@
 import { useState } from 'react';
 import { Navbar, Nav, NavDropdown, Container, Col, Row } from 'react-bootstrap';
 import styles from '../styles/Menubar.module.css';
+import  BrandLogo  from './brandlogo';

 export default function Menubar(props) {
  const [collapsed, setCollapsed] = useState(true);

  return (
    <Container fluid class='border-bottom '>
      <Navbar expand='lg' className=' bg-white mx-4 my-2'>


Please lint your code.

RonLek · 2022-01-23T18:35:03Z

app/styles/BrandLogo.module.css

+.brand {
+    font-size: clamp(16px, 2.4vw, 20px);
+}
+


Remove extra lines (lint your code)

RonLek · 2022-01-23T18:35:30Z

app/components/menubar.js

+        <BrandLogo 
+          brandLink={'#home'}
+          logoLink={'https://global-uploads.webflow.com/611a19b9853b7414a0f6b3f6/611bbb87319adfd903b90f24_logoRC.svg'}
+          imageTitle={'Rocket.chat'}


Rocket.Chat

RonLek · 2022-01-23T19:17:00Z

@samad-yar-khan please also squash your commits into one.

samad-yar-khan · 2022-01-23T19:51:48Z

@RonLek Sir, I have linted my code squashed the commits into one.

Sing-Li · 2022-01-23T20:07:57Z

@Sing-Li Can you look into this ? I read the docs regarding NextJS packing and lazy loading , I can incorporate the feature to only load the logo image when its viewed to increase efficiency but I don't think this would be much useful for this particular component as the navbar will always be loaded first and so will the brand logo. We can surely utilize lazy loading with some of the larger graphics components which are shown as we scroll further down. Let me know if I am thinking in the right direction :)

I've made comment in-line. When designing a NextJS component - you need to anticipate when and where it will be used. For example, some Community Builder may have complex logo image/svg that takes minutes to render and the rest of the page where the component is used may have hundreds of other images/svgs to load. You are thinking too narrowly focused on ONLY THIS use-case with our Rocket.Chat logo/branding (and only as part of the navbar).

Sing-Li · 2022-01-23T20:12:17Z

app/components/brandlogo.js

+export default function BrandLogo(props) {
+  return (
+    <Navbar.Brand href={props.brandLink} className={styles.brand}>
+      <img


Consider using the Image component from NextJS.

samad-yar-khan · 2022-01-23T21:10:20Z

dLink} className={styles.brand}>

I understand, looking at the broader scope it does make sense to use the lazy loading feature. I will use the component , from NextJS .I just started learning NextJS today, so didn't really know that I could use that. Thanks a lot for the suggestion 💯 and thankyou for rectifying issues in the PR. I will make the requested changes 👍

samad-yar-khan · 2022-01-23T23:18:24Z

@Sing-Li I have made the BrandLogo component lazy loading such that, the logo is only fetched and displayed when we scroll over the component. I have used the following package : https://www.npmjs.com/package/react-cool-inview. Let me if there are any changes.

Sing-Li · 2022-01-24T19:51:09Z

app/components/brandlogo.js

@@ -0,0 +1,28 @@
+import dynamic from "next/dynamic";


Please do not use dynamic or inview. These are pure unnecessary baggages.

Just using <Image> will get you all the benefits as stated in its docs. Please read the doc.

I understand, I will remove dynamic and inview and use the properties of <Image> for optimisation

samad-yar-khan · 2022-01-24T23:00:45Z

@Sing-Li I have removed the unnecessary code. and packages I went through the documentation and figured that by default the <Image> component is Lazy Loading, so the logo will only be fetched and loaded on view. Thankyou for the suggestion :)

irffanasiff · 2022-05-22T09:07:26Z

@samad-yar-khan i was trying to remove the warnings in the console, there was this warning which is due to brandlogo component can you please look into it

samad-yar-khan · 2022-05-22T10:50:29Z

Sing-Li merged commit 2be69ef into RocketChat:master on Jan 25
2 checks passed

@demonicirfan I will just look into it

Sing-Li reviewed Jan 23, 2022

View reviewed changes

Sing-Li requested changes Jan 23, 2022

View reviewed changes

Sing-Li assigned samad-yar-khan Jan 23, 2022

RonLek requested changes Jan 23, 2022

View reviewed changes

samad-yar-khan force-pushed the Fix_header_log branch from fac0347 to 2061afd Compare January 23, 2022 19:50

Sing-Li changed the title ~~Set Header Image~~ BrandLogo Component Jan 23, 2022

Sing-Li requested changes Jan 23, 2022

View reviewed changes

samad-yar-khan force-pushed the Fix_header_log branch 2 times, most recently from 1fbab1b to 4efd378 Compare January 23, 2022 23:11

Sing-Li requested changes Jan 24, 2022

View reviewed changes

Make LazyLoading Brand Logo Component

ce6f80e

samad-yar-khan force-pushed the Fix_header_log branch from 4efd378 to ce6f80e Compare January 24, 2022 22:50

Sing-Li approved these changes Jan 25, 2022

View reviewed changes

Sing-Li merged commit 2be69ef into RocketChat:master Jan 25, 2022

sidmohanty11 mentioned this pull request Mar 27, 2022

[BUG] Unable to load the community logo #95

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

BrandLogo Component #46

BrandLogo Component #46

samad-yar-khan commented Jan 23, 2022 •

edited by RonLek

Loading

CLAassistant commented Jan 23, 2022 •

edited

Loading

Sing-Li left a comment

Sing-Li left a comment •

edited

Loading

Sing-Li left a comment

samad-yar-khan commented Jan 23, 2022

samad-yar-khan commented Jan 23, 2022

RonLek Jan 23, 2022

RonLek Jan 23, 2022

RonLek Jan 23, 2022

RonLek commented Jan 23, 2022

samad-yar-khan commented Jan 23, 2022

Sing-Li commented Jan 23, 2022 •

edited

Loading

Sing-Li Jan 23, 2022 •

edited

Loading

samad-yar-khan commented Jan 23, 2022

samad-yar-khan commented Jan 23, 2022

Sing-Li Jan 24, 2022

samad-yar-khan Jan 24, 2022

samad-yar-khan commented Jan 24, 2022

irffanasiff commented May 22, 2022

samad-yar-khan commented May 22, 2022

BrandLogo Component #46

BrandLogo Component #46

Conversation

samad-yar-khan commented Jan 23, 2022 • edited by RonLek Loading

Proposed changes (including videos or screenshots)

Issue(s)

Steps to test or reproduce

Before :

After :

CLAassistant commented Jan 23, 2022 • edited Loading

Sing-Li left a comment

Choose a reason for hiding this comment

Sing-Li left a comment • edited Loading

Choose a reason for hiding this comment

Sing-Li left a comment

Choose a reason for hiding this comment

samad-yar-khan commented Jan 23, 2022

samad-yar-khan commented Jan 23, 2022

RonLek Jan 23, 2022

Choose a reason for hiding this comment

RonLek Jan 23, 2022

Choose a reason for hiding this comment

RonLek Jan 23, 2022

Choose a reason for hiding this comment

RonLek commented Jan 23, 2022

samad-yar-khan commented Jan 23, 2022

Sing-Li commented Jan 23, 2022 • edited Loading

Sing-Li Jan 23, 2022 • edited Loading

Choose a reason for hiding this comment

samad-yar-khan commented Jan 23, 2022

samad-yar-khan commented Jan 23, 2022

Sing-Li Jan 24, 2022

Choose a reason for hiding this comment

samad-yar-khan Jan 24, 2022

Choose a reason for hiding this comment

samad-yar-khan commented Jan 24, 2022

irffanasiff commented May 22, 2022

samad-yar-khan commented May 22, 2022

samad-yar-khan commented Jan 23, 2022 •

edited by RonLek

Loading

CLAassistant commented Jan 23, 2022 •

edited

Loading

Sing-Li left a comment •

edited

Loading

Sing-Li commented Jan 23, 2022 •

edited

Loading

Sing-Li Jan 23, 2022 •

edited

Loading