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

Improvement/60 light dark themes #103

Merged
merged 8 commits into from Jan 17, 2019
Merged

Conversation

vifor2
Copy link
Contributor

@vifor2 vifor2 commented Jan 17, 2019

This PR adds a secondary (dark) theme that can be toggled on by clicking on the slider after opening the settings' drop-down menu. The theme is applied instantly and reapplied if needed each time the .html report is opened.

The visual of both themes isn't definitive since @j4v brought to my attention that #59 should've been completed prior to #60. Therefore I'll make the required adjustments to #60 after completing #59, which includes new bootstrap 4 themes.

Here's a collage of the default theme (renamed to light) and dark theme :
image

@Aboisier Aboisier added this to In progress in Scout Suite via automation Jan 17, 2019
@Aboisier Aboisier removed this from In progress in Scout Suite Jan 17, 2019
Copy link
Contributor

@Aboisier Aboisier left a comment

Choose a reason for hiding this comment

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

Nice improvement, it looks great!

I'm not sure if we plan on adding other themes in the future. If we do, we could make the code and the UI a bit more scalable without too much effort I believe.

*/
function set_theme(file)
{
var oldlink = document.getElementsByTagName("link").item(THEME_INDEX);
Copy link
Contributor

@Aboisier Aboisier Jan 17, 2019

Choose a reason for hiding this comment

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

I think getting the link element holding the theme with an id instead of with an index would be more reliable. This solution breaks if the lines are moved around or if we add another link tag before.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think a light & dark themes are sufficient, but it would be nice if the implementation allows to easily swap themes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Now using an index, thanks for the tip @Aboisier ! @j4v Currently the only work needed to swap out themes is swapping inc-bootstrap/css/bootstrap-dark.min.css for a different file of the right version (3 atm) and changing the value of the const DARK_THEME in scoutsuite.js accordingly.

ScoutSuite/output/data/inc-scoutsuite/scoutsuite.js Outdated Show resolved Hide resolved
Copy link
Collaborator

@x4v13r64 x4v13r64 left a comment

Choose a reason for hiding this comment

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

Not sure if this is a local issue but switching themes has no effect:

sc_2019-01-17_10h28m19s

Also seems like the BS is broken? e.g. Dashboard should be blue.

There's also a scoutsuite.css file that gets dropped in the scoutsuite-report folder.

@vifor2
Copy link
Contributor Author

vifor2 commented Jan 17, 2019

Not sure if this is a local issue but switching themes has no effect:

After a clean clone + execution I now see the issue you mention. When working on the project I modified files in /scoutsuite-report in order to skip the process of making a report each time, I must've poorly transfered over my changes to the /ScoutSuite. Will fix.

There's also a scoutsuite.css file that gets dropped in the scoutsuite-report folder.

EDIT: Found the cause.

@codecov-io

This comment has been minimized.

@nccgroup nccgroup deleted a comment from codecov-io Jan 17, 2019
@vifor2 vifor2 merged commit 5a54a57 into develop Jan 17, 2019
@Aboisier Aboisier deleted the improvement/60-light-dark-themes branch January 19, 2019 21:25
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

5 participants