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

[WIP] [Don't Merge] Use component header #361

Closed

Conversation

brian-smith-tcril
Copy link
Contributor

all of the existing header functionality is now working with the shared header component, however, the styles in this PR don't fully match up with the existing header

looking for direction/feedback on what needs to happen for this header style-wise (do i need to override a ton of bootstrap classes?), after that is sorted i'll clean up the code side of things and get it ready to merge

@ihor-romaniuk
Copy link
Contributor

Hi @brian-smith-tcril.
Do you need any help to speed up the merger of this PR and backport these fixes to the olive release?

@brian-smith-tcril
Copy link
Contributor Author

@ihor-romaniuk there's still the open question of

looking for direction/feedback on what needs to happen for this header style-wise (do i need to override a ton of bootstrap classes?), after that is sorted i'll clean up the code side of things and get it ready to merge

i don't really see the benefit of using a shared component if that also means using a lot of app specific css overrides. ideally the header component style would make sense "out of the box" everywhere it's being used, and provide easy paths for customization (and not require extra overrides for app specific overrides)

@arbrandes arbrandes requested a review from connorhaugh January 30, 2023 13:37
@arbrandes
Copy link
Contributor

@connorhaugh, can we get a pair of eyes on this one? Thanks!

@connorhaugh
Copy link
Contributor

connorhaugh commented Jan 30, 2023

looking for direction/feedback on what needs to happen for this header style-wise

I can look into finding exact mockups, screenshots of existing implementations, or something like that? My team could also do a UI review ourselves if that is what is required? What set of resources would solve this open question?

@brian-smith-tcril
Copy link
Contributor Author

brian-smith-tcril commented Jan 30, 2023

The questions I had were more along the lines of "what are we allowed to change?"

The existing header in course authoring is using styles that mimic the non-mfe studio styles, this shared component does not mimic those styles (see library-authoring post openedx-unsupported/frontend-app-library-authoring#74)

@arbrandes
Copy link
Contributor

@brian-smith-tcril, it would be cool if we can whip up comparison screenshots, in any case.

@brian-smith-tcril
Copy link
Contributor Author

@arbrandes from olive-demo

Course Authoring:
Screenshot from 2023-01-30 13-59-04

Library Authoring:
image

@brian-smith-tcril
Copy link
Contributor Author

the header component is currently utilizing a paragon actionrow https://paragon-openedx.netlify.app/components/actionrow/

it might make sense to migrate to a navbar https://paragon-openedx.netlify.app/components/navbar/

@connorhaugh
Copy link
Contributor

connorhaugh commented Jan 31, 2023

@brian-smith-tcril Yeah so the issue is that course authoring bar looks a little different than lib-authoring. Here is a current screenshot from prod rn. We'd like to maintain feature parity, but obviously keeping up with requirements and dependancies is important too. Thanks so much! (obv this has edx theme on it)
Screen Shot 2023-01-31 at 4 34 56 PM

@connorhaugh
Copy link
Contributor

connorhaugh commented Jan 31, 2023

"what are we allowed to change?"

Don't remove any feature functionality. We will have new designs for studio, maybe eventually.

@KristinAoki
Copy link
Member

@brian-smith-tcril This repo is now using a header from frontend-component-header. Is this PR still relevant or should it be closed?

@brian-smith-tcril
Copy link
Contributor Author

@KristinAoki that's awesome! I'll go ahead and close this.

@brian-smith-tcril brian-smith-tcril deleted the use-component-header branch February 1, 2024 16:06
bradenmacdonald pushed a commit to open-craft/frontend-app-authoring that referenced this pull request Aug 9, 2024
fix: resolve  openedx#360 make style consistent across outlines
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.

5 participants