-
-
Notifications
You must be signed in to change notification settings - Fork 14
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
update ui of error display #131
base: main
Are you sure you want to change the base?
Conversation
hi, So i wanted to ask can i work on rewamping of the UI |
From a general PoV, yes, everyone's welcomed. More specifically, you are welcomed to either ask for assignment of a specific issue which is already opened, or create new issues if you have suggestions. When you come with a new idea, it is always way better to open an issue, wait for feedback and only then write code. It is too easy to miss something (especially when you join a new project) and write code for "nothing". That been said, thank you for this first PR, I'll have a look |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- can you share screenshot on smaller screen size as well?
- please update CHANGELOG.md (add one entry in "Unreleased" section with this PR number (normally we reference the issue, but since there is no issue for this PR, then we use PR number)
- the text font seems pretty weird, is it really then proper font? did you started the UI with
yarn dev
as explained at https://github.com/openzim/kolibri/blob/main/CONTRIBUTING.md#developing-the-zim-ui-in-vuejs ?
and the most important/urgent issues to tackle are listed in https://github.com/openzim/kolibri/milestone/8 ; process is: select one issue, ask for assignment in the issue, and start working on it |
1d7a9ea
to
125fe25
Compare
Why did you dropped the title instead of replacing it with proper value as suggested? Please at least explain / comment, just submitting a new review without at least a comment on why you did things differently than suggested does not help. |
So i was not able to properly understand that so i removed it as there was no title before. |
I'm sorry, but the fact that every point I raise does not get any real answer but just causes you to follow my suggestion does not help me to have much trust in the whole PR. One part I've only briefly reviewed is the bunch of new CSS statements you've added. I will spend some time to review them one-by-one to better understand how they are all needed or not. I unfortunately do not have lots of bandwidth, might take some time |
It's completely okay if you close the PR. I'll work on some different issue. |
This PR is not a problem, the changes you've made looks good from a UI perspective and we need to move this forward (at least I want to). The problem is that we need to be sure that all code changes made by a PR are correct, we cannot just live on "UI looks good", the project needs to be maintainable on the long run. And for this, we need to all be sure that all code changes introduced by a PR are appropriate. We usually expect committers to be sure that what they are proposing is sound. In this PR, the points I raised too often finished in something were you finally recognized you didn't really know if these changes were required or not. Especially the font thing which I raised quite soon in the PR and finished by you removing the added code line. Nothing to be ashamed of, it is acceptable to not be sure of what we are doing, but it means I need to be even more diligent in my review of your PRs. |
I got your point, I was not ashamed or anything. I learned what to keep in mind and check everything. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Other than the body
CSS directives which seems useless to me, I can now confirm this looks good.
Please squash all commits into one and force push to your branch, so that we have only one commit to merge, no need to have multiple commits for this small change.
Updated error display to modern UI and made it responsive Updated error display to modern UI Updated error display to modern UI and made it responsive Commit unstaged changes before rebasing
-updated the error display to modern ui
-made the error message responsive