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

Settings Panel created and made fully functional #35

Merged
merged 5 commits into from Dec 16, 2020

Conversation

vinaysrivastava273
Copy link
Contributor

@vinaysrivastava273 vinaysrivastava273 commented Dec 12, 2020

All the 4 features of the Settings Panel are now fully functional. Cookies have been deployed to work on both the pages ( index.html and user.html) and the appearance is now decent.

Currently,with Settings Panel, we can change the time format and username.
The Fonts list is now available to the end user for changing the Greeting message. Howeve, the Seetings Panel doesn't look good currently.
Currently, all the 4 features of Settings panel are working. However, there is some glitch with its appearance.
@vinaysrivastava273
Copy link
Contributor Author

vinaysrivastava273 commented Dec 12, 2020

There are some problems with this Settings Panel for which I need help.

  1. The moment one clicks Font Style menu, the menu gets enlarged due to a few font styles having a very long name. I have tried making the menu uniform by enlarging the Settings width. However, the changes weren't very pleasing.
  2. The Font list gets appended every time one clicks on the 'Font Style' menu (i.e. After 'Z' fonts, fonts starting with 'A' gets added below).
    I have tried adding 'removeEventListener('click',function)'. However this removes the click event even before the user selects the font style.
  3. Also, suggest the changes you would like to see in the appearance of the Panel.
  4. There's a "Unexpected token :" error popping up in the console. But, I couldn't figure how to resolve it.
  5. Every time you open a new tab, the Background color and the font style gets changed back to default. I am working on this currently.

@sudo-rgorai
Copy link
Owner

Hey @vinaysrivastava273. This is amazing work. Do not focus too much on appearance right now as there's a lot of of things going on here. Let's try to make everything functional and then we can maybe raise another issue on design.

As far as the problems you're facing, I analysed your code briefly and shall address each of them point wise below.

  1. For Fonts style menu enlargement, try using width: 100%; for #fonts in your index.css and see if it works.
  2. The second issue can be solved by using a simple if statement . In /src/font_list.js the for-loop where you're appending options (line 17), should be inside an if-statement. The if-statement should check something like select.children.length < 1024. This should prevent your options from getting appended every time the user clicks on Font Styles menu.
  3. As far as appearance is concerned, as I said earlier, it's okay if you don't focus too much on it right now and skip this part if you want to.
  4. The Unexpected token is a bit weird. JSON file seems okay. The way you're handling it also seems fine. I think we can ignore it for now as it does not break anything.
  5. The settings revert back to initial state across various sessions because you're not using persistent storage. The JavaScript variables that you're using to store data remains active only in your current session and gets reloaded every time you reload the page. Luckily we do have a persistent storage at our disposal. Cookies! We have already used cookies for storing user's name (See /src/index.js). We can also do that for color and font.

Try these and post here if there's any more issues.
Great work so far. Kudos! 😄

I have added cookies to remember the user's all prefernces(time format, font, background color). Also, the font list is appended only once. The appearance has been optimized to look decent.
@vinaysrivastava273
Copy link
Contributor Author

Hey @sudo-rgorai , I have made all the required changes to Settings panel. Kindly check them and suggest if you want any further changes. Learnt quite a lot of things in this single issue. 😄

@sudo-rgorai
Copy link
Owner

sudo-rgorai commented Dec 16, 2020

I'm very glad that you had an opportunity to learn. We're all here to learn ☺️ . The PR seems alright now. And apart from minor issues, LGTM. 👍

PS: Apologies for the late reply. Got occupied elsewhere.

@sudo-rgorai sudo-rgorai merged commit 9279f87 into sudo-rgorai:master Dec 16, 2020
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

2 participants