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

Add about page #55

Merged
merged 10 commits into from Jul 18, 2022
Merged

Add about page #55

merged 10 commits into from Jul 18, 2022

Conversation

rzvl
Copy link
Contributor

@rzvl rzvl commented Jul 11, 2022

Changes

closes #51

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

Checklist

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings

@rzvl
Copy link
Contributor Author

rzvl commented Jul 11, 2022

Do I need to add styles too?

@getify
Copy link
Owner

getify commented Jul 11, 2022

I wouldn't worry about any different styles yet... all that will come once we start designing. We do however want some basic markup for the nav, and that markup should be the same in both/all .html files.

@rzvl
Copy link
Contributor Author

rzvl commented Jul 11, 2022

Great. Then I would add nav markup too...

Copy link
Owner

@getify getify left a comment

Choose a reason for hiding this comment

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

The diff on index.html has a LOT of changes, mostly formatting related it seems. Are you using an editor plugin/setting that re-formats code?

If so, can you please disable that and reapply your changes to the original file so we're not seeing such an extensive diff of mostly irrelevant changes.

@rzvl
Copy link
Contributor Author

rzvl commented Jul 13, 2022

Apparently prettier had automatically formatted the file when I saved it. I just disabled prettier and reformatted both, 'index.html' and 'about.html', files. I tried to follow the formatting style which had been used in original 'index.html' file.

I hope it's correct!

Copy link
Owner

@getify getify left a comment

Choose a reason for hiding this comment

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

Pretty close, but a few changes we need to address, as noted inline.

web/about.html Outdated Show resolved Hide resolved
web/index.html Show resolved Hide resolved
web/about.html Show resolved Hide resolved
web/about.html Outdated Show resolved Hide resolved
@rzvl
Copy link
Contributor Author

rzvl commented Jul 13, 2022

Thanks for your review and great hints! I just updated the files accordingly. Please check if I have done the innert handling part correctly.

@pselle pselle requested a review from getify July 15, 2022 18:45
web/js/main.js Outdated Show resolved Hide resolved
web/js/main.js Outdated Show resolved Hide resolved
... it's only used in this one function.
Copy link
Owner

@getify getify left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for making the changes!

@getify
Copy link
Owner

getify commented Jul 17, 2022

Just FYI... gonna hold off merging this because there's another PR that's made some changes to the notification-manager.js file, and I want to make sure we avoid merge conflicts. So we'll try to merge those together once the other PR is ready.

Copy link
Owner

@getify getify left a comment

Choose a reason for hiding this comment

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

Good to go!

@getify getify merged commit bca8172 into getify:main Jul 18, 2022
@rzvl rzvl deleted the about-page branch July 19, 2022 09:42
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.

Add About page
2 participants