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

Js scss reformttings #167

Merged
merged 44 commits into from
Jul 31, 2022
Merged

Js scss reformttings #167

merged 44 commits into from
Jul 31, 2022

Conversation

CommanderStorm
Copy link
Member

These are the changes to the js/scss files, to make #112 more reviewable

@CommanderStorm CommanderStorm changed the base branch from main to ci July 19, 2022 17:29
@CommanderStorm CommanderStorm requested a review from octycs July 19, 2022 17:29
@CommanderStorm CommanderStorm self-assigned this Jul 19, 2022
webclient/src/core.js Outdated Show resolved Hide resolved
@CommanderStorm CommanderStorm force-pushed the js-scss-reformttings branch 2 times, most recently from c6df327 to f4e8bbc Compare July 29, 2022 12:31
Copy link
Contributor

@octycs octycs left a comment

Choose a reason for hiding this comment

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

I have tried to look over all files. Sometimes the diff failed to understand the changes and hard to read, so I hope I haven't missed anything that it not reformatting.

There is only one thing that I think we need to keep, that is the navigatum.cloneState function instead of structuredClone, because Samsung Internet doesn't support it yet. Alternatively we could add a polyfill into core.js so that it is always available (even if polyfills.js is not loaded)

webclient/gulpfile.js Show resolved Hide resolved
webclient/src/history-states.js Show resolved Hide resolved
webclient/src/history-states.js Outdated Show resolved Hide resolved
webclient/package.json Outdated Show resolved Hide resolved
webclient/src/variables.scss Outdated Show resolved Hide resolved
webclient/src/variables.scss Outdated Show resolved Hide resolved
webclient/src/variables.scss Outdated Show resolved Hide resolved
webclient/src/variables.scss Outdated Show resolved Hide resolved
webclient/src/core.js Outdated Show resolved Hide resolved
@CommanderStorm CommanderStorm requested a review from octycs July 30, 2022 23:06
@CommanderStorm
Copy link
Member Author

I noiced that the gulpfile has stopped processing spectre. I will look into this on monday :)

@octycs
Copy link
Contributor

octycs commented Jul 31, 2022

I noiced that the gulpfile has stopped processing spectre. I will look into this on monday :)

Just to be sure, have you done npm update after the change in the package.json 😅

Edit: I see, sorry, same problem for me now.

@octycs
Copy link
Contributor

octycs commented Jul 31, 2022

It was related to the border-color variables in sass and should be fixed now.

@CommanderStorm CommanderStorm changed the base branch from ci to main July 31, 2022 17:37
Signed-off-by: Frank Elsinga <[email protected]>
Signed-off-by: Frank Elsinga <[email protected]>
This is more consistent with the style used across most of the project

Signed-off-by: Frank Elsinga <[email protected]>
This way is more consistent across the existing codebase

Signed-off-by: Frank Elsinga <[email protected]>
Reasoning is, that it seems a bit more readable and browser support is there: https://caniuse.com/mdn-css_types_color_rgb_alpha_parameter

Signed-off-by: Frank Elsinga <[email protected]>
Signed-off-by: Frank Elsinga <[email protected]>
added stylelint-scss to the plugins

Signed-off-by: Frank Elsinga <[email protected]>
CommanderStorm and others added 27 commits July 31, 2022 19:53
TODO find out if this works
Reason:
for..in loops iterate over the entire prototype chain, which is virtually never what you want. Use Object.{keys,values,entries}, and iterate over the resulting array
.forEach is superiour to for..of due to Polyfills and other issues
- only things that modern browsers use are getting delivered are listed as dependencys.
- polyfills/build tooling is listed as below the other Dependencies, because while included in prod, they are only delivered to a minority of users
@CommanderStorm CommanderStorm merged commit 6e16862 into main Jul 31, 2022
@CommanderStorm CommanderStorm deleted the js-scss-reformttings branch July 31, 2022 18:00
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