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.

Sign up for GitHub

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

Jump to bottom

Restyled the components #42

Merged
merged 2 commits into from
Jan 22, 2022
Merged

Restyled the components #42

merged 2 commits into from
Jan 22, 2022

Conversation

irffanasiff
Copy link
Contributor

@irffanasiff irffanasiff commented Jan 18, 2022 •
edited
Loading

Solves issue #20

Completed Tasks

  • Website looks the same as the design and is responsive.
  • Responsive carousel.
  • Used Bootstrap for most of the part.

Remaining Tasks

  • I am working on Navbar, I am trying to make it look as similar to the design as I can, it will take some time.
  • I was not able to use custom colors in bootstrap so I used CSS for colors. I will figure it out and use custom colors it will reduce the amount of CSS.
  • Consistent text styles and sizes on the webpage are still to be done. Will do that with colors.
  • Used dummy data in the community activity section.

@CLAassistant
Copy link

CLAassistant commented Jan 18, 2022 •
edited
Loading

CLA assistant check
All committers have signed the CLA.

@lgtm-com
Copy link

lgtm-com bot commented Jan 18, 2022

This pull request introduces 4 alerts and fixes 4 when merging d729eba into a5ca2af - view on LGTM.com

new alerts:

  • 4 for Unused variable, import, function or class

fixed alerts:

  • 4 for Unused variable, import, function or class

@Sing-Li
Copy link
Member

Sing-Li commented Jan 18, 2022 •
edited
Loading

I am working on Navbar, I am trying to make it look as similar to the design as I can, it will take some time.

Awesome. Thanks for submitting it WIP early. This will really help with the review.

Regarding the Navbar --- please be aware that we will be adding two sub-components to it very shortly:

  1. rightmost will be a square auth component -- interchangeably it can be supported by Firebase/auth0/gluu/keycloak component(s)
  2. second right most will be a "3 dots drop down" Superprofile component

@RonLek can DM you with a live sample upon request.

@irffanasiff
Copy link
Contributor Author

Ok, so we can style it with CSS because bootstrap does not provide much flexibility for customization. Later on we can add those two sub-components.

@Sing-Li
Copy link
Member

Sing-Li commented Jan 18, 2022 •
edited
Loading

@demonicirfan I ran a test on the checkout of the PR. Everything looks good.

image

The one change I'd request is for the search drop-down to be on the left of the searchbar (even when reactive). And the Magnifying glass be on the right of the search entry.

Thanks! Please let me know when the next stable checkpoint is reached on the PR.

Sing-Li
Sing-Li reviewed Jan 19, 2022
View reviewed changes
app/components/discourserankedlist.js Outdated
class='bi bi-hand-thumbs-up'
viewBox='0 0 16 16'
>
<path d='M8.864.046C7.908-.193 7.02.53 6.956 1.466c-.072 1.051-.23 2.016-.428 2.59-.125.36-.479 1.013-1.04 1.639-.557.623-1.282 1.178-2.131 1.41C2.685 7.288 2 7.87 2 8.72v4.001c0 .845.682 1.464 1.448 1.545 1.07.114 1.564.415 2.068.723l.048.03c.272.165.578.348.97.484.397.136.861.217 1.466.217h3.5c.937 0 1.599-.477 1.934-1.064a1.86 1.86 0 0 0 .254-.912c0-.152-.023-.312-.077-.464.201-.263.38-.578.488-.901.11-.33.172-.762.004-1.149.069-.13.12-.269.159-.403.077-.27.113-.568.113-.857 0-.288-.036-.585-.113-.856a2.144 2.144 0 0 0-.138-.362 1.9 1.9 0 0 0 .234-1.734c-.206-.592-.682-1.1-1.2-1.272-.847-.282-1.803-.276-2.516-.211a9.84 9.84 0 0 0-.443.05 9.365 9.365 0 0 0-.062-4.509A1.38 1.38 0 0 0 9.125.111L8.864.046zM11.5 14.721H8c-.51 0-.863-.069-1.14-.164-.281-.097-.506-.228-.776-.393l-.04-.024c-.555-.339-1.198-.731-2.49-.868-.333-.036-.554-.29-.554-.55V8.72c0-.254.226-.543.62-.65 1.095-.3 1.977-.996 2.614-1.708.635-.71 1.064-1.475 1.238-1.978.243-.7.407-1.768.482-2.85.025-.362.36-.594.667-.518l.262.066c.16.04.258.143.288.255a8.34 8.34 0 0 1-.145 4.725.5.5 0 0 0 .595.644l.003-.001.014-.003.058-.014a8.908 8.908 0 0 1 1.036-.157c.663-.06 1.457-.054 2.11.164.175.058.45.3.57.65.107.308.087.67-.266 1.022l-.353.353.353.354c.043.043.105.141.154.315.048.167.075.37.075.581 0 .212-.027.414-.075.582-.05.174-.111.272-.154.315l-.353.353.353.354c.047.047.109.177.005.488a2.224 2.224 0 0 1-.505.805l-.353.353.353.354c.006.005.041.05.041.17a.866.866 0 0 1-.121.416c-.165.288-.503.56-1.066.56z' />
Copy link
Member

Choose a reason for hiding this comment

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

Please move all the in-line SVG to app/public. We'll need a strategy to keep them with the associated component in the longer run.

Sing-Li
Sing-Li reviewed Jan 19, 2022
View reviewed changes
app/components/personalcircle.js
className={`${styles.personas} d-flex flex-wrap justify-content-center`}
>
<span className={`${styles.persona}`}>
<div className={styles.svg}>
Copy link
Member

Choose a reason for hiding this comment

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

pls move in-line svg as per earlier comment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In the previous code, we were getting all the data used here ( like name and icons ) from backend, so we can replace the svg's there with these svg's directly and change the frontend code when that is done

@irffanasiff
Copy link
Contributor Author

@demonicirfan I ran a test on the checkout of the PR. Everything looks good.

image

The one change I'd request is for the search drop-down to be on the left of the searchbar (even when reactive). And the Magnifying glass be on the right of the search entry.

Thanks! Please let me know when the next stable checkpoint is reached on the PR.

i have redesigned it you can have a look at the Figma file now and suggest if we should go with that. I have sent updates in the navbar-considerations chat

@irffanasiff
Copy link
Contributor Author

I will add all the svg's to /public folder while working on navbar. For now i have added these two svg's

@lgtm-com
Copy link

lgtm-com bot commented Jan 19, 2022

This pull request introduces 2 alerts and fixes 5 when merging 4dd3991 into a5ca2af - view on LGTM.com

new alerts:

  • 2 for Unused variable, import, function or class

fixed alerts:

  • 5 for Unused variable, import, function or class

Sing-Li
Sing-Li approved these changes Jan 22, 2022
View reviewed changes
Copy link
Member

@Sing-Li Sing-Li left a comment

Choose a reason for hiding this comment

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

Good enough for initial commit.

@Sing-Li Sing-Li merged commit 43d489e into RocketChat:master Jan 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Sing-Li Sing-Li Sing-Li approved these changes

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
@irffanasiff @CLAassistant @Sing-Li