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

Notification drawer heading template is wrapped in h4 #539

Open
benjaminapetersen opened this issue Aug 1, 2017 · 11 comments
Open

Notification drawer heading template is wrapped in h4 #539

benjaminapetersen opened this issue Aug 1, 2017 · 11 comments

Comments

@benjaminapetersen
Copy link
Member

benjaminapetersen commented Aug 1, 2017

The current notification-drawer.html template wraps the user defined heading.html (from headingInclude) in an h4, which is restrictive for styling beyond a simple heading.

In progress web console PR here.

Here is where we are going with the openshift web-console:

screen shot 2017-08-01 at 10 04 26 am

From notification-drawer.html it would be ideal to eliminate the h4. If preferred for out of the box styling, move it inside the heading.html example template:

        <div class="panel-heading">
          <h4 class="panel-title">
            <a ng-if="!$ctrl.singleGroup" ng-click="$ctrl.toggleCollapse(notificationGroup)" ng-class="{collapsed: !notificationGroup.open}" ng-include src="$ctrl.headingInclude"></a>
            <span ng-if="$ctrl.singleGroup" ng-include src="$ctrl.headingInclude"></span>
          </h4>
          <span class="panel-counter" ng-include src="$ctrl.subheadingInclude"></span>
        </div>

@jeff-phillips-18 thoughts?

@jeff-phillips-18
Copy link
Member

we will need to look into backwards compatibility issues that could arise from making this change but I can understand the reasoning.

@dtaylor113
Copy link
Member

Could we eliminate the h4 and update .panel-title and set font-weight and size to match what an h4 would be? Then apps could overwrite the .panel-title class.

@jeff-phillips-18
Copy link
Member

We could. The issue would be any applications already overriding this using the h4 as the selector.

@benjaminapetersen
Copy link
Member Author

How closely is angular-patternfly trying to follow SemVer? Strictly no breaking changes until next major release?

@jeff-phillips-18
Copy link
Member

jeff-phillips-18 commented Sep 19, 2017

Yes, that is what we are trying for.

@benjaminapetersen
Copy link
Member Author

So Angular (2) seems to follow SemVer, but is on version 4.4.2 with 5.0.0-beta.7 coming up soon. They seem to bump the major version semi-frequently, so the SemVer signal "something breaks" is there, but its not a big delayed rollout.

Curious if that would work for angular-patternfly. Following SemVer is great if 1.) you can go ahead and release the major as often as you have a breaking change (even if minor like an h4), or 2.) if code changes are so infrequent that its ok to wait a relatively long time for a major release.

I'm guessing its hard to put enough architectural time into each new component to ensure tweaks (breaking tweaks) are avoided.

@jeff-phillips-18
Copy link
Member

Yes, that is the struggle. I don't see the h4 really causing enough of an issue to warrant making the change and creating a major release. Is this something that you feel can not be (easily) worked around?

@benjaminapetersen
Copy link
Member Author

Agree. Not a problem for now, I think we have an acceptable workaround in place in the console.

@jeff-phillips-18
Copy link
Member

OK, closing this issue. Feel free to reopen if you'd like it kept in the backlog for the next major release.

@benjaminapetersen
Copy link
Member Author

I'd probably prefer to keep it open. Its definitely still strange to work around the heading.

@jeff-phillips-18
Copy link
Member

OK, thanks @benjaminapetersen

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

No branches or pull requests

3 participants