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

Changes to Home Page Formatting #147

Closed
wants to merge 8 commits into from
Closed

Conversation

Pulathisi
Copy link

@Pulathisi Pulathisi commented Oct 14, 2019

Description

Changing the home page formatting.

  • New feature (non-breaking change which adds functionality)

How Has This Been Tested?

yarn test done. All tests passed

Checklist:

  • I have performed a self-review of my own code

background-color: white;
padding: 1rem 0 1rem 0;
}
// #mission {
Copy link
Member

@SudharakaP SudharakaP Oct 14, 2019

Choose a reason for hiding this comment

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

You don't need to comment them; just delete them if they are unnecessary. 😄

@SudharakaP
Copy link
Member

SudharakaP commented Oct 14, 2019

@Pulathisi : Thanks for the changes 😄

Can you please fill out the description of the PR with what it does and change the heading too into what the PR is about. 😄

Also I saw that you've added a package-lock.json file. This means you've used npm. However the project uses yarn. Could you remove that file and use yarn instead. Yarn works almost the same way as npm; https://github.com/python-sinhala-education-society/OpenLearnr/blob/master/docs/DEVELOPER_DOCUMENTATION.md; except you'll get a yarn.lock file.

@@ -36,6 +36,7 @@
"stripe-angular": "0.5.0",
"swagger-ui": "2.2.10",
"tslib": "1.10.0",
"yarn": "^1.19.1",
Copy link
Member

@SudharakaP SudharakaP Oct 14, 2019

Choose a reason for hiding this comment

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

This is unnecessary. The generator of this project (JHipster) auto installs yarn locally. If you go to your root folder of the project you'll see a npm directory and within it a yarn directory (provided that you build the project with ./mvnw).

Copy link
Author

Choose a reason for hiding this comment

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

yarn suggested to do 'yarn audit autofix' to fix some yarn irregularities. I did that and yarn has added this.

/* navbar */

.jh-navbar {
background-color: #222035 !important;
Copy link
Member

Choose a reason for hiding this comment

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

Can we not use this !important annotation when making changes, but rather directly change .jh-navbar and .hipster. It makes it difficult to maintain the code in the future; https://css-tricks.com/when-using-important-is-the-right-choice/

Copy link
Author

Choose a reason for hiding this comment

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

changed css to navbar.scss and deleted overrides.css file ;) change come in the next sync

@SudharakaP
Copy link
Member

@Pulathisi : I tested your change and it seems to remove some formatting from the front page. Also the mobile formatting seems off.

image

whereas the original one was,

image

Also the change seems to make the titles and text centered;

image

whereas the original was,

image

Normally card text and titles are left aligned (https://material.io/components/cards/#). I try to maintain the Google Material Design guide; so can we go by it for these kind of changes. 😄

Can we not do this change for now machn? I don't see a huge improvement to the front page. One way we could improve it though is making it more in accordance with material design guidelines.

@SudharakaP SudharakaP changed the title Pulathisi ui feature Changes to Home Page Formatting Oct 14, 2019
@Pulathisi
Copy link
Author

Pulathisi commented Oct 19, 2019

@Pulathisi : I tested your change and it seems to remove some formatting from the front page. Also the mobile formatting seems off.

image

whereas the original one was,

image

Also the change seems to make the titles and text centered;

image

whereas the original was,

image

Normally card text and titles are left aligned (https://material.io/components/cards/#). I try to maintain the Google Material Design guide; so can we go by it for these kind of changes. 😄

Can we not do this change for now machn? I don't see a huge improvement to the front page. One way we could improve it though is making it more in accordance with material design guidelines.

@SudharakaP LOL is this material. I thought its flat design. Let me know if you want a material touch to it. U have to add shadows and stuff

https://en.wikipedia.org/wiki/Flat_design

Will check the why the quote went missing :D

Loader makes the site look like a game. reminds us of PACMAN 😄 Shall I change it to something meaningful. Like that of a book folding or something related to education ?

Will change code to make it match for mobile devices as well.

@Pulathisi
Copy link
Author

@SudharakaP I force pushed my changes related to home page. Let me know if there is any issues with that ;) check for mobile devices now. I did some ui modifications. Also please note that I didnt check for Ipads. Only mobiles

@SudharakaP
Copy link
Member

@Pulathisi : Hey, machn, sorry; can we not do this change for now? The reason is our front page and stuff is okay for now and it works without issue; surely it can be improved but there's more pressing issues with the site than changing the look of the front page. I'll keep this in mind and we'll revisit this later if you are okay with that. 😄

It's my fault actually and I should have been more clear of the process; sorry if it wasted your time. Normally what I do is this; I go to issues (https://github.com/python-sinhala-education-society/OpenLearnr/issues) and create a new issue with the changes I want to do. Sometimes @asanka-herath will do that too. Also I normally add the appropriate badges and include it in our project board (https://github.com/python-sinhala-education-society/OpenLearnr/projects/2). And when I have time I go to this project board and take an issue and work on it. This is how normally most open source projects work. The issue is for discussing the change and then we do the pull request if it's agreed to do it.

How about this? For front end stuff, there's lots of things to do actually. One of the things that @asanka-herath has problems with is searching for videos from the comment approval screen. We have an issue for that; #51 . Also #116 is a good improvement that could make things easier for admins. In addition the following two issues that I just created; #154, #155 could improve user experience for the admin/instructors greatly. If you have any questions about these feel free to comment on those issues. Also feel free to create any new issues if you have new ideas. 😄 I am sure there's lots of this kind of issues; and I'll create some more in the days to come.

The aesthetic appeal of the website is okay for now. We lack lots of features such as notifications when a comment is made; and comment editing #55 and after we do these we'll come back to polishing the UI aspects.

@Pulathisi
Copy link
Author

@SudharakaP Its now I realized what you really intend me to do. 😄 I will work on the issues you are referring to but since you are saying the UI can be improved I doubt why you don't go ahead with these changes. For me the mobile UIs and Desktop UIs have improved with my changes and I don't see any reason to reject these changes. If front page stuff is okay but surely can be improved shall we accept these changes and move on from here ?

@SudharakaP
Copy link
Member

SudharakaP commented Oct 20, 2019

@SudharakaP Its now I realized what you really intend me to do. smile I will work on the issues you are referring to but since you are saying the UI can be improved I doubt why you don't go ahead with these changes. For me the mobile UIs and Desktop UIs have improved with my changes and I don't see any reason to reject these changes. If front page stuff is okay but surely can be improved shall we accept these changes and move on from here ?

The reason is, trying to merge this we'll have to do few more iterations of testing and corrections. For example the CI is failing with this change. I like us to work on things that being currently are problems for users (such as the ones I described in my previous comment). And I feel we have more pressing matters to address than doing visual changes right now and keep iterating on this issue wastes both our time. Does that make sense? 😄 😄

Also if you don't like the issues I mentioned above please feel free to just open an issue with your ideas then we could discuss about them. 😄

We could definitely discuss about these kind of changes once we make the site a bit more user friendly in terms of it's functionality. The visual appearance is a matter of perspective and it's hard to make a compelling argument in either direction on what it should really look like. 😄

@SudharakaP
Copy link
Member

@Pulathisi : I am going to close this issue for now; I've added some more info to our readme and contributions pages about opening an issue before making pull requests. This way in the future we can all be in the same page. 😄

@SudharakaP SudharakaP closed this Oct 21, 2019
@Pulathisi
Copy link
Author

@SudharakaP If that's the case I am not the one suitable for this project. 😄 Because front end look and feel is what I enjoy working with. So best of luck with your project. Wish you guys strength to continue the good work. 😄

@SudharakaP
Copy link
Member

@Pulathisi : How about this. If you like to work on front end UI changes can you open an issue on how it can be improved, maybe even linking to this PR? Is this PR a larger change you anticipate to get the site in accordance with some sort of design you have in mind? 😄

The problem I am thinking is with the PR first approach it's harder to understand what the change achieves. What I am suggesting is exactly what we do for example in this project; https://github.com/jhipster/generator-jhipster.

@SudharakaP SudharakaP deleted the pulathisi-ui-feature branch May 23, 2020 18:36
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