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

got rid of bootstrap.min.js #500

Open
wants to merge 17 commits into
base: next
Choose a base branch
from
Open

got rid of bootstrap.min.js #500

wants to merge 17 commits into from

Conversation

astraindev
Copy link

@astraindev astraindev commented Aug 22, 2019

Prerequisites

Recommended Steps

  • My patch adds a new feature, therefore I have also added a help article about it
  • My patch changes Elegant behavior, therefore I have updated the help article to reflect this change
  • My commits are signed

Description

Obviously, the custom.css file text will need to be merged into elegant.css. I just didn't want to screw up upstream pull requests.
This was referenced Aug 22, 2019
@talha131 talha131 self-assigned this Aug 22, 2019
@talha131
Copy link
Member

@andrewdstrain couple of feedbacks

  1. Use hooks https://elegant.oncrashreboot.com/use-pre-commit-git-hooks
  2. Follow guidelines https://elegant.oncrashreboot.com/git-commit-guidelines

Feature parity

  • clicking on an accordion should close it if it is already open
  • CSS style is very different

I suggest taking advantage of unused CSS tools to extract the relevant CSS from bootstrap.

Also, have a look at this snippet. It is much simpler and cleaner

https://codepen.io/abergin/pen/ihlDf

comments are done
javascript should still work but needs testing
@astraindev
Copy link
Author

astraindev commented Aug 23, 2019

By me doing CSS only, the JavaScript would need to make sure that the checkbox is checked when already open. Then it would close when already open. If it's simply shown but not checked, the first click will check the checkbox, which opens everything.

In custom.css, I did get rid of all the tag selectors. It's only using bootstrap.min.css selectors now.

I did remove the disqus.comment.count class because the counts never work - at least for me - even without all of these changes (on master). That class is used by the JavaScript from Disqus - not you. So it's not really your problem. But getting rid of it does fix it.

I've never done a pull request before. But I installed the hooks and am trying to follow the guidelines. If I'm not doing that right, please let me know what I'm doing wrong.

I agree - the other snippet is simpler and cleaner. I'm going to look into that.

I really appreciate this. Thank you so much!

@talha131
Copy link
Member

the JavaScript would need to make sure that the checkbox is checked when already open. Then it would close when already open. If it's simply shown but not checked, the first click will check the checkbox, which opens everything.

JS is not needed. As you can see in the snipper I shared.

I've never done a pull request before. But I installed the hooks and am trying to follow the guidelines. If I'm not doing that right, please let me know what I'm doing wrong.

No issues. Take your time. In case anything is not clear, feel free to post here to contact me on my email talha131@gmail.com.

@astraindev
Copy link
Author

astraindev commented Aug 25, 2019

I looked at the code at CodePen and it's doing pretty much the same thing as I'm doing. Once you remove all the display related CSS, it looks like this: https://www.w3schools.com/code/tryit.asp?filename=G7C87MRRMZ4R.

See the grocery list? It doesn't display as it should. That's because only HTML elements are used in the CSS.

Thus, you need to use classes. Once you use classes, you don't need the elements in the CSS and can change the elements in the HTML if needed. So look at this: https://www.w3schools.com/code/tryit.asp?filename=G7C8JCTKRP8G.

The Grocery list is displayed as normal and the HTML elements are different but everything still works. The two URL links (above) are identical except that elements are used like in the website you showed me and classes are used in the one I changed. Everything else is the same.

And don't think that I cheated with the grocery list by changing the elements in the HTML. Look at this: https://www.w3schools.com/code/tryit.asp?filename=G7C8N8WUZUN4.

You can clearly see that the HTML elements match now and the grocery list displays as it should. This is because the CSS is based on classes and not elements.


There are some minor differences in what the one you showed me does and what I posted to you. One difference is not needing the label element. Another is that the code I posted works with both radio and checkboxes. If you look in the categories.html change, you'll see that I used radio to make the accordion to work like, well, an accordion. They both (radio and checkbox) use the same CSS selectors. Of course, the other difference is that this code uses checked to indicate a no-show. That's the reason it won't work with radio inputs. You could add different CSS selectors for radio, though, which would get things working.

I got rid of the i element in the examples I posted because it's only used for the up/down arrow on the right, which none of your accordion stuff has in the Elegant theme. I'm trying to keep things as close to looking the same because it's a theme and that would change the look of the theme. But it's obviously your call.

I also got rid of the cool animations - that's only to keep the examples simple because what we're talking about is how the accordion works, not how cool it looks. All that stuff can be added back in.

@astraindev
Copy link
Author

Having said all of that, I am going to change the CSS to follow the CodePen example. I highly agree that it’s easier to read and follow.

@talha131
Copy link
Member

  1. Let me know if you are facing any problem with Git hooks. Your feedback would help us update the Git hooks help article. (@jackdewinter)
    I asked because I see you are still failing the hook test.
  2. You re right. The classes version is better because it gives us more flexibility.

I'm trying to keep things as close to looking the same because it's a theme and that would change the look of the theme.

This is very important. I am glad we both are on the same page here.

Bug

  1. Click on a category, say, "Connecting with your reader"
  2. It would be open
  3. Click on it again
  4. Nothing happens.

Expected result: It would close.

Next step

Match the CSS with bootstrap. Here is how to do it.

  1. Get un-minified Bootstrap 2.3.2 CSS from here https://getbootstrap.com/2.3.2/
  2. Create a very simple Html page that just has this accordion. https://getbootstrap.com/2.3.2/javascript.html#collapse. See an example below.
  3. Test accordion is working inside a browser
  4. Use an online un-used CSS server. Upload Html page and CSS to it
  5. It would remove all the CSS that is redundant and give you back the relevant CSS.
  6. Place that CSS in a new file like "accordion.css". (See how adominitions.css is added to learn)
  7. Add it to Elegant

Now, the new categories should match the old ones in look and feel. It will also take care of the animation, hopefully.

<html>
<body>
<div class="accordion" id="accordion2">
  <div class="accordion-group">
    <div class="accordion-heading">
      <a class="accordion-toggle" data-toggle="collapse" data-parent="#accordion2" href="#collapseOne">
        Collapsible Group Item #1
      </a>
    </div>
    <div id="collapseOne" class="accordion-body collapse in">
      <div class="accordion-inner">
        Anim pariatur cliche...
      </div>
    </div>
  </div>
  <div class="accordion-group">
    <div class="accordion-heading">
      <a class="accordion-toggle" data-toggle="collapse" data-parent="#accordion2" href="#collapseTwo">
        Collapsible Group Item #2
      </a>
    </div>
    <div id="collapseTwo" class="accordion-body collapse">
      <div class="accordion-inner">
        Anim pariatur cliche...
      </div>
    </div>
  </div>
</div>
</body>
</html>

@astraindev
Copy link
Author

That's a feature, not a bug! (joke)

It's because the radio button doesn't uncheck like checkboxes do. There's another way to do the CSS. It doesn't use input type radio but anchors instead. It suffers from the same bug because once the anchor is clicked on, clicking on it again just selects the same anchor.

@talha131
Copy link
Member

Right. Then let's use checkboxes (like here https://codepen.io/abergin/pen/ihlDf) and use CSS classes.

@talha131
Copy link
Member

@andrewdstrain is the PR ready now?

@astraindev
Copy link
Author

astraindev commented Nov 9, 2019

@andrewdstrain is the PR ready now?

As far as I can tell, yes.

I'm also getting rid of Bootstrap CSS. I'm now merging all the custom.css changes into the elegant.css file. Right now, I'm on the step of getting rid of CSS that is no longer needed in the elegant.css file. The next step is to merge the changes in custom.css into elegant.css.

You can check out the changes using the Inspect Element and View Page Source features of your web browser on my website Geeking Out With. You can also look at the GitHub for the not minimized CSS changes.

You (or someone else) might be able to help me with this as my time is a bit less than it used to be.

@talha131
Copy link
Member

@andrewdstrain I just tried the deploy preview.

https://deploy-preview-500--pelicanelegant.netlify.com/categories

The categories are not working. Clicking on each do change the URL but does not expand the accordion.

Let me know if you can look into it, otherwise, I will fix the issue on top of your PR.

@talha131
Copy link
Member

talha131 commented Feb 2, 2020

The categories are not working. Clicking on each do change the URL but does not expand the accordion.

@andrewdstrain any update on this? If you can fix the issue I stated above, then I will fix the CSS style issue myself.

@hno2
Copy link

hno2 commented Aug 9, 2020

What is the status on this Request, is there anything I can help with?

@talha131
Copy link
Member

talha131 commented Aug 9, 2020

@hno2

  1. There are conflicts
  2. Look wise it does not resemble what we currently have
  3. The expand does not work
  4. It does not following the current project structure

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

4 participants