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

Some little changes to website #20

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Some little changes to website #20

wants to merge 4 commits into from

Conversation

ghost
Copy link

@ghost ghost commented Dec 12, 2021

  • Correct GitHub spelling.
  • Update admins list.
  • Drop unused domain variable.

Hendra Manudinata and others added 3 commits December 12, 2021 20:05
Signed-off-by: Hendra Manudinata <[email protected]>
Updated on: December 12 2021 GMT+7.

Signed-off-by: Hendra Manudinata <[email protected]>
Signed-off-by: Hendra Manudinata <[email protected]>
@ghost
Copy link
Author

ghost commented Dec 12, 2021

The domain variable one may needs a little extra caution. AFAIK, that doesn't needed. CMIIW!

Also, please don't merge for now! There's/re some more changes, but probably tomorrow. It's been 20:30, need rest from code.

Reduce the headache of adding three lines just to add an admin.

Note that this produces some dirty indentation in the built HTML
because of the tags usage itself. Maybe need some workaround
for this.

Signed-off-by: Hendra Manudinata <[email protected]>
@ghost
Copy link
Author

ghost commented Dec 13, 2021

Let me know what you're thinking about this @ammarfaizi2 @irvanmalik48.

@ammarfaizi2
Copy link
Contributor

The domain variable one may needs a little extra caution. AFAIK, that doesn't needed. CMIIW

All the detail should go into the patch itself, GitHub is just a side channel, it's perfectly fine to write it in the PR section, but the commit is more important than this PR thread. So put that explanation in the commit message.

The git tree is the source of truth for figuring out why a particular change was done and needed. Not GitHub!

<li>
<a class="a-white" href="https://t.me/rin4th"
>@rin4th</a
>
</li>

Copy link
Contributor

@ammarfaizi2 ammarfaizi2 Dec 13, 2021

Choose a reason for hiding this comment

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

You have extra trailing whitespace here.

Copy link
Author

Choose a reason for hiding this comment

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

Yes, as mentioned in the commit.

The workaround is to add the for tags after the <ol> and </li> (endfor). But that'll make the source code ugly.

Copy link
Contributor

@ammarfaizi2 ammarfaizi2 Dec 13, 2021

Choose a reason for hiding this comment

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

There are more

Applying: sns: Correct `GitHub` spelling
Applying: home: Update Admin list
Applying: config: Drop unused `domain` variable
Applying: home: Use liquid `for` loop for admin list
.git/rebase-apply/patch:127: trailing whitespace.
            
.git/rebase-apply/patch:133: trailing whitespace.
            
.git/rebase-apply/patch:139: trailing whitespace.
            
.git/rebase-apply/patch:145: trailing whitespace.
            
.git/rebase-apply/patch:151: trailing whitespace.
            
warning: squelched 7 whitespace errors
warning: 12 lines add whitespace errors.

@ammarfaizi2
Copy link
Contributor

If we use an auto-generated admin list at all, the previous commit updating admin list is not needed, please drop that.

@ghost
Copy link
Author

ghost commented Dec 13, 2021

The domain variable one may needs a little extra caution. AFAIK, that doesn't needed. CMIIW

All the detail should go into the patch itself, GitHub is just a side channel, it's perfectly fine to write it in the PR section, but the commit is more important than this PR thread. So put that explanation in the commit message.

The git tree is the source of truth for figuring out why a particular change was done and needed. Not GitHub!

Okay, I'll add it and repush. Sorry.

@ghost
Copy link
Author

ghost commented Dec 13, 2021

If we use an auto-generated admin list at all, the previous commit updating admin list is not needed, please drop that.

You're right -- Dropping it after lunch.

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.

1 participant