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

explorer-client: improve linting rules #506

Merged
merged 2 commits into from
Mar 7, 2022
Merged

Conversation

pchrysochoidis
Copy link
Contributor

  • no binding for jsx attrs
  • no duplicate imports
  • group scss/css imports together after type imports

resolves #480

Copy link
Contributor

@apburnie apburnie left a comment

Choose a reason for hiding this comment

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

This adds feedback from the other PR. We should get PR 471 through first (#471). We can then check how this eslint config plays with the website.

@@ -9,10 +10,19 @@ module.exports = {
['internal', 'parent', 'sibling', 'index'],
'type',
],
pathGroups: [
{
pattern: '{.,..}/**/*.?(s)css',
Copy link
Contributor

Choose a reason for hiding this comment

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

We do not need scss handling. As in other PR complexity should be avoided unless it is needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let's leave it there since it's there already, even if we remove scss or add them later, there will be no need to visit this again

Copy link
Contributor

Choose a reason for hiding this comment

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

yes, let's remove this. Anything unused should always be removed

@pchrysochoidis
Copy link
Contributor Author

This adds feedback from the other PR. We should get PR 471 through first (#471). We can then check how this eslint config plays with the website.

If we agree that the rules are correct, I don't see any reason adding code that might not comply and then fixing it.

@apburnie
Copy link
Contributor

Team discussions suggest that the top priority is the demo at the end of March. Being pessimistic, we could potentially present a demo with a mock data backend. We cannot present a demo where the UI is incomplete. Hence, for now, the focus needs to be on website functionality.

After the tight deadline has passed, we can then shift attention to refactoring and optimizations. This PR is part of this process. After the demo deadline, it would also be of value to have a team meeting to set refactoring priorities. That meeting is likely to feed into this PR and to shape what rules we use going forwards.

Copy link
Contributor Author

@pchrysochoidis pchrysochoidis left a comment

Choose a reason for hiding this comment

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

This is now rebased to latest main

@@ -9,10 +10,19 @@ module.exports = {
['internal', 'parent', 'sibling', 'index'],
'type',
],
pathGroups: [
{
pattern: '{.,..}/**/*.?(s)css',
Copy link
Contributor

Choose a reason for hiding this comment

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

yes, let's remove this. Anything unused should always be removed

@stella3d
Copy link
Contributor

stella3d commented Mar 7, 2022

This looks good, but i agree we should remove SCSS handling.

* no binding for jsx attrs
* no duplicate imports
* group scss/css imports together after type imports
* also rename index.scss to index.css (leftover from scss removing)
@pchrysochoidis pchrysochoidis force-pushed the pc-480-improve-linting branch from 2522d9f to d86d1af Compare March 7, 2022 19:25
@stella3d stella3d dismissed apburnie’s stale review March 7, 2022 19:30

changes requested have been done

@stella3d stella3d merged commit d442f64 into main Mar 7, 2022
@stella3d stella3d deleted the pc-480-improve-linting branch March 7, 2022 19:30
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.

[explorer-client] Improve linting
4 participants