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

Pop-up and Options page #14

Closed
wants to merge 15 commits into from
Closed

Pop-up and Options page #14

wants to merge 15 commits into from

Conversation

realyuvishere
Copy link

  • What does this PR do?
    The pop-up window has been improved with a little bit of responsive behaviour, along with font-family changes all over the doc. The options section has also been modified.
  • Any background context you want to provide?
    Addresses issue Improve the popup sheet #1 and Improve the options page #9 .
  • Screenshots and/or Live Demo
    demo

@prateek3255
Copy link
Owner

This seems nice, but the options page needs to be a separate page because it can also be accessed from the extension menu.
image

But I really liked your idea, so I would suggest moving the options code to popup. And rename it something like Recently Saved Scrrolls.

Also just use something like this to reverse the object, so that we have the recently saved URLs first. And slice it to show the top 5 URLs, since we are going to show all of them in the options page anyway.

@realyuvishere
Copy link
Author

sure thing! I had something similar in mind, just needed some validation.

@prateek3255
Copy link
Owner

@yuvrajsharma07 I have updated the master branch so please update your branch accordingly. Also, let me know if you're stuck anywhere.

# Conflicts:
#	options.js
#	save.js
@realyuvishere
Copy link
Author

Yeah! I have even styled the options page, added some bar indicating the percentage as well. Will do the popup object reverse thing and commit soon. Here is a snippet
Screenshot 2019-10-08 at 3 17 35 PM

Copy link
Owner

@prateek3255 prateek3255 left a comment

Choose a reason for hiding this comment

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

Please remove these conflict indicators

options.js Outdated Show resolved Hide resolved
options.js Outdated Show resolved Hide resolved
options.js Outdated Show resolved Hide resolved
save.js Outdated Show resolved Hide resolved
save.js Outdated Show resolved Hide resolved
save.js Outdated Show resolved Hide resolved
@prateek3255
Copy link
Owner

Also, I am planning to go forward with #15 for the options page

@realyuvishere
Copy link
Author

realyuvishere commented Oct 9, 2019

Yeah, completely forgot about that file.

Modified the options page to maintain design consistency with the popup, otherwise it's your call :)

Please check the code for the reverse object.

Pushed the items to an array, reversed it and displayed only the first 5.

# Conflicts:
#	options.css
#	popup.css
#	popup.html
#	popup.js
@prateek3255
Copy link
Owner

You did not removed the merge conflict indicators again.

options.css Outdated


>>>>>>> devfolioco/master
Copy link
Owner

Choose a reason for hiding this comment

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

☝️

@realyuvishere
Copy link
Author

demo 1
A demo of the few changes i made in the popup, the reverse object isnt too prominent, because of the issue #42, but it is functional, you may modify the options page as you may feel like :)

I took the liberty of using a different font to make it all look better, do remark on that too

@realyuvishere
Copy link
Author

@prateek3255

# Conflicts:
#	popup.html
@prateek3255
Copy link
Owner

Moving in a different direction, so won't be able to merge it for now. Thanks for experimenting with this.

@prateek3255 prateek3255 closed this Oct 1, 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.

2 participants