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.

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

Theme konnectd #698

Merged
merged 22 commits into from
Oct 19, 2020
Merged

Theme konnectd #698

merged 22 commits into from
Oct 19, 2020

Conversation

LukasHirt
Copy link
Contributor

@LukasHirt LukasHirt commented Oct 14, 2020

Bring konnectd frontend, create ownCloud theme and generate assets.

image

Open tasks:

  • Changelog item
  • Replace placeholder logo with the ownCloud one when generating
  • Bring ownCloud favicon when generating

@LukasHirt LukasHirt self-assigned this Oct 14, 2020
@update-docs
Copy link

update-docs bot commented Oct 14, 2020

Thanks for opening this pull request! The maintainers of this repository would appreciate it if you would create a changelog item based on your changes.

@LukasHirt
Copy link
Contributor Author

There are some parts of the frontend which are probably not required but were causing a lot of errors when removed. I'd like to go in a separate PR through them when there'd more time and do some followup cleanup.

@LukasHirt
Copy link
Contributor Author

@micbar Have you got some idea why this line is not respected by codacy https://github.com/owncloud/ocis/pull/698/files#diff-3404650b2859b064f8b0527d200936d6b306cf8e11437f0051b38af5aeaab1c2R4 and the error in webpack config is still showing up?

@micbar
Copy link
Contributor

micbar commented Oct 14, 2020

@LukasHirt this file is no longer used.

Use the top level one and adapt the paths

@micbar
Copy link
Contributor

micbar commented Oct 14, 2020

same for the .gitignore

@kulmann
Copy link
Member

kulmann commented Oct 15, 2020

@LukasHirt please rebase to current master. A grpc-ecosystem package that we are using in some Makefiles has been renamed => already adjusted on master.

@LukasHirt
Copy link
Contributor Author

please rebase to current master. A grpc-ecosystem package that we are using in some Makefiles has been renamed => already adjusted on master.

Done 😉

@LukasHirt
Copy link
Contributor Author

Use the top level one and adapt the paths

Still getting the error even though I moved it in there 😞

.codacy.yml Outdated Show resolved Hide resolved
Copy link
Member

@kulmann kulmann left a comment

Choose a reason for hiding this comment

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

Noticed some things:

  • file konnectd/ui/src/images/owncloud-logo.svg is part of this PR and should be removed or some placeholder image, right?
  • There are some files from the original kopano theme (augmenting teamwork, etc) which could be removed
  • The font size of the input fields is too small. It should be 1rem, but is 0.8...rem. With the current size it looks weird coming from the login form with tiny font size to ownCloud Web with quite big font size.

@LukasHirt
Copy link
Contributor Author

file konnectd/ui/src/images/owncloud-logo.svg is part of this PR and should be removed or some placeholder image, right?

Right, thanks!

There are some files from the original kopano theme (augmenting teamwork, etc) which could be removed

I removed leftover images for now but regarding other files is this comment:

There are some parts of the frontend which are probably not required but were causing a lot of errors when removed. I'd like to go in a separate PR through them when there'd more time and do some followup cleanup.

The font size of the input fields is too small. It should be 1rem, but is 0.8...rem. With the current size it looks weird coming from the login form with tiny font size to ownCloud Web with quite big font size.

Per request from the marketing team, I have adjusted this according to the ownCloud website form inputs where it is the same font size.

@kulmann
Copy link
Member

kulmann commented Oct 19, 2020

The font size of the input fields is too small. It should be 1rem, but is 0.8...rem. With the current size it looks weird coming from the login form with tiny font size to ownCloud Web with quite big font size.

Per request from the marketing team, I have adjusted this according to the ownCloud website form inputs where it is the same font size.

Hm... I don't agree. 😅 Styling the login page should be consistent with phoenix, not the website. 👀 We're going back and forth between phoenix and the login page, the website is never part of this process.

If styles of the input elements should be adjusted in general, that's a different story. But breaking style consistency within a user workflow is a no go IMO.

Copy link
Member

@refs refs left a comment

Choose a reason for hiding this comment

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

First of all, huge PR 👏 I took a look mostly at the react components, leaving build stuff out, and just a matter of consistency and a suggestion for using TypeScript 🤓 . Haven't tried the PR locally, intending to do so later today.

konnectd/ui/src/components/Loading.js Outdated Show resolved Hide resolved
@@ -0,0 +1,93 @@
import React from 'react';
import PropTypes from 'prop-types';
import { connect } from 'react-redux';
Copy link
Member

Choose a reason for hiding this comment

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

haven't worked with Vuex, only with redux, but wouldn't it be better to use vuex + react instead of adding another store? Curious :D

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This would create extra dependency on Vue, wouldn't it? At least when looking at https://www.npmjs.com/package/react-vuex that's the feeling I am getting

}
}

Goodbyescreen.propTypes = {
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

@LukasHirt LukasHirt Oct 19, 2020

Choose a reason for hiding this comment

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

I'd very much love that! Not sure if I can introduce in one PR not only react but TS as well though 🙈 Maybe that'd be too much?

konnectd/ui/src/containers/Login/Chooseaccount.js Outdated Show resolved Hide resolved
@LukasHirt
Copy link
Contributor Author

Addressed your comments @refs and increased font size and changed the height of inputs to reflect the other ones in ocis-web @kulmann Thank you both for reviews! I've rebased the PR also so hopefully the random API tests failures will be gone.

Add new line at the end of file

Shorten hex

Add zero
Copy link
Member

@kulmann kulmann left a comment

Choose a reason for hiding this comment

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

🚀

@kulmann
Copy link
Member

kulmann commented Oct 19, 2020

love it!

@sonarcloud
Copy link

sonarcloud bot commented Oct 19, 2020

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities (and Security Hotspot 0 Security Hotspots to review)
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@LukasHirt LukasHirt merged commit a2823a0 into master Oct 19, 2020
@LukasHirt LukasHirt deleted the branded-kopano branch October 19, 2020 22:32
ownclouders pushed a commit that referenced this pull request Oct 19, 2020
Merge: eb31510 b8f64ce
Author: Lukas Hirt <[email protected]>
Date:   Tue Oct 20 00:32:05 2020 +0200

    Merge pull request #698 from owncloud/branded-kopano
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.

None yet

4 participants