Skip to content

Simplify EsHeaderComponent implementation. #303

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

Closed
wants to merge 12 commits into from
Closed

Simplify EsHeaderComponent implementation. #303

wants to merge 12 commits into from

Conversation

alexeykostevich
Copy link

  • Update the component to use Ember Octane API.
  • Remove the code for reseting expanded on CSS breakpoints for
    a more predictable behaviour and prevention of memory leaks.
  • Add navbar-expanded class to simplify styles and reduce coupling
    with HTML structure.
  • Fix accessibility warnings.
  • Add additional tests.

mansona and others added 10 commits March 31, 2020 10:23
- Update the component to use Ember Octane API.
- Remove the code for reseting `expanded` on CSS breakpoints for
  a more predictable behaviour and prevention of memory leaks.
- Add `navbar-expanded` class to simplify styles and reduce coupling
  with HTML structure.
- Fix accessibility warnings.
- Add additional tests.
highlighted when expanded.
@mansona
Copy link
Member

mansona commented Apr 13, 2020

wow thanks for the contribution 🎉 I had a quick glance and it looks mostly ok, although I'm not sure we can use @tracked quite yet because of the versions of apps that will be consuming the styleguide 🤔 I'll have to get back to you on that one.

I'm concerned merging this into the update branch because I would much prefer to merge this into master once update is merged and the website-redesign-rfc branch is merged. There is a PR that is currently blocking this whole process #301 but I'm hopeful that we can get it unblocked this week 👍

I just wanted to let you know that there will be a delay getting this merged but it will likely get merged once that delay is fixed 🎉

@alexeykostevich
Copy link
Author

Thank you for looking at this and sharing the details, @mansona ! It sound great: there is no rush at all with this PR.

I'm all for getting the master branch ready first. Frankly, currently, it takes time for new newcomers to find the right place to start. #301 will make the repository even more contributor-friendly -- great job!

Please, let me know if any help from my side is needed.

@mansona mansona force-pushed the upgrade branch 2 times, most recently from d685368 to 0cd8b45 Compare April 20, 2020 21:26
@alexeykostevich alexeykostevich changed the base branch from upgrade to master April 26, 2020 16:17
@alexeykostevich
Copy link
Author

@mansona , I've update the PR target to master. Please, let me know if you have any concerns about the PR.

@mansona
Copy link
Member

mansona commented Apr 29, 2020

@alexeykostevich can you rebase this instead of merging 🤔 the history seems a tiny bit out of whack

Let me know if you have any questions about the rebase 👍 I'm happy to help if you need it

@alexeykostevich
Copy link
Author

That's a very good point! I've created another PR (#319) with a 1-commit history.

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