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

create github action workflow for linting html #154

Merged
merged 5 commits into from
May 15, 2023

Conversation

Roshanjossey
Copy link
Member

address: #142

@Roshanjossey Roshanjossey requested review from jonatoni, sleepypioneer and a team May 6, 2023 16:31
Copy link
Member

@jancborchardt jancborchardt left a comment

Choose a reason for hiding this comment

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

I am probably not too knowledgeable about this so excuse the possibly silly question @Roshanjossey :) – should the lint not run through properly on this pull request then? As it seems it failed.

@sleepypioneer
Copy link
Member

I am probably not too knowledgeable about this so excuse the possibly silly question @Roshanjossey :) – should the lint not run through properly on this pull request then? As it seems it failed.

The failing one is the old circle CI linter. The new one doesn't appear to have run here.

@Roshanjossey
Copy link
Member Author

I tried this in my fork before raising a PR here. Roshanjossey#3

Lint action runs there and fails with expected error messages.

My theory about it not running here is that there is higher security restriction here to prevent workflows in PRs running

@sleepypioneer
Copy link
Member

I see the following
Screenshot 2023-05-08 at 21 35 05

I selected option 3 : allow opensourcediversity ....

@sleepypioneer
Copy link
Member

sleepypioneer commented May 8, 2023

probably we need some change, maybe close and reopen this PR? reading here I think that would do it

@Roshanjossey Roshanjossey reopened this May 8, 2023
@sleepypioneer
Copy link
Member

sleepypioneer commented May 9, 2023

hmm still not running 🤔 maybe we can change it to on push? I would have to read up more on which of those options that we can choose from for actions should be selected perhaps it needs to be less restictive

runs-on: ubuntu-latest
steps:
- name: Checkout code
uses: actions/checkout@v2
Copy link
Member

Choose a reason for hiding this comment

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

any reason to not use v3?

Copy link
Member Author

Choose a reason for hiding this comment

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

No, updated to v3

@sleepypioneer
Copy link
Member

I changed the settings for including github created actions (somehow thought this would have been default 🤪 )

@sleepypioneer
Copy link
Member

ok sorry @Roshanjossey I got there now 😅 The setting I chose did not include github created actions, I noticed here the following:

--

Error: .github#L1
actions/checkout@v2 is not allowed to be used in opensourcediversity/opensourcediversity.org. Actions in this workflow must be: within a repository owned by opensourcediversity.

@sleepypioneer
Copy link
Member

now it runs but fails because of the following reasons:

line 195 column 119 - Warning: unescaped & or unknown entity "&I"
line 196 column 53 - Warning: unescaped & or unknown entity "&I"
line 34 column 33 - Warning: trimming empty
line 35 column 68 - Warning: trimming empty
line 357 column 41 - Warning: proprietary attribute "dnt"

as it's not very many do you want to fix them here?

@Roshanjossey
Copy link
Member Author

Thank you for following up @sleepypioneer. Ended up fixing #152 too when fixing lint errors

<div class="animated-icon"><span></span></div>

<button class="navbar-toggler first-button" type="button" data-toggle="collapse" data-target="#navbar" aria-controls="navbar" aria-expanded="false" aria-label="Toggle navigation">
<span class="screen-reader-text">Menu</span>
Copy link
Member

Choose a reason for hiding this comment

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

is this a new thing? Seems like a good thing but I am not very familiar with this code. Not sure if this was related to a failing HTML linter or if seemed appropriate to add this in here?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is a re-implementation of hamburger button for menu in mobile width that got removed in #138

This has to be handled to pass lint because the empty button and span tags trigger warnings from tidy.

I decided not to revert to old changes and implement it in a different way because this would be a more semantic HTML way to do this.

Also, interestingly, this bug may have been introduced to fix lint errors (as described in #123). So, we're coming full circle.

Copy link
Member

Choose a reason for hiding this comment

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

thanks for clarifying 🙏 I could certainly do with a refresh of the code base!

@sleepypioneer sleepypioneer self-requested a review May 10, 2023 22:39
Copy link
Member

@sleepypioneer sleepypioneer left a comment

Choose a reason for hiding this comment

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

thank you for taking this on @Roshanjossey, awesome improvement

Copy link
Member

@jancborchardt jancborchardt left a comment

Choose a reason for hiding this comment

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

Thanks a bunch, also for all the fixes! :) Let's get this in

@jancborchardt jancborchardt merged commit 8e98a4d into main May 15, 2023
1 check passed
@jancborchardt jancborchardt deleted the github-action-lint branch May 15, 2023 10:19
This was referenced May 15, 2023
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

3 participants