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

feat: centred and styled productsNavbar #585

Merged
merged 38 commits into from
Jul 5, 2022
Merged

feat: centred and styled productsNavbar #585

merged 38 commits into from
Jul 5, 2022

Conversation

Kinukeo
Copy link
Contributor

@Kinukeo Kinukeo commented Mar 4, 2022

Description

Buttons on the navbar will now change depending on which wrapper is selected, these buttons also now allow the user to navigate to other wrappers.

relates #306

Checklist:

Put an x in the boxes that apply to this pull request (you can also fill these out after opening the pull request). If you're unsure about any of these, don't hesitate to leave a comment on this pull request!

  • I have read the gliff.ai Contribution Guide.
  • I have requested to pull a branch and not from main.
  • I have checked all commit message styles match the requested structure.
  • My code follows the style guidelines of this project.
  • I have commented my code, particularly in hard-to-understand areas.
  • I have performed a self-review of my own code.
  • I have assigned 3 or less reviewers.
  • New and existing unit tests pass locally with my changes.
  • I have added tests that prove my fix is effective or that my feature works.
  • My changes generate no new warnings.
  • I have made corresponding changes to the documentation.
  • New database changes have been committed.
  • If appropriate, I have bumped any version numbers.

@Kinukeo Kinukeo requested review from a team, ChasNelson1990 and ChrisBaidoo and removed request for a team March 4, 2022 17:31
@gliff-ai-robot gliff-ai-robot added the enhancement [Improvement] Enhancement request. label Mar 4, 2022
@Kinukeo Kinukeo changed the title feat: update navbar layout between wrappers feat: update navbar layout between wrappers WIP Mar 4, 2022
@ChasNelson1990
Copy link
Member

Some first run through visual comments:

  • This appears to be the default navbar lol:
    image
    It's what I see when I am logged out but not on the sign in/up pages and also on /manage/projects
  • If I click the little cat thing the entire app crashes
  • "Navigate to AUDIT" actually takes me to /manage/projects in the account I tested with - this is because it is an essentials account so actually doesn't get AUDIT at all!

Looks like a good start though! I'll look through the code at a later point.

@Kinukeo Kinukeo marked this pull request as ready for review April 18, 2022 08:39
@Kinukeo Kinukeo changed the title feat: update navbar layout between wrappers WIP feat: centred and styled productsNavbar Apr 18, 2022
@Kinukeo Kinukeo requested a review from philipjackson April 21, 2022 09:03
Copy link
Contributor

@joshuajames-smith joshuajames-smith left a comment

Choose a reason for hiding this comment

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

Good job on getting this together, it all appears to work for me!
I just have a couple styling changes:

  • Could we make the devider lines (marked by the red box) reach the top and bottom?
  • Could we add some more space between the centre text and these divider lines?
  • The text is currently in all caps, could we have this set as normal?

Screenshot 2022-04-21 at 10 30 17

Also a question. I'm sure not if this is in another pr/issue, but we have lost the current product view and help button?

Screenshot 2022-04-21 at 10 30 30 vs Screenshot 2022-04-21 at 10 54 09

Finally just a side note, I know we have plans to remove this page from shown via a redirect (#595) but just to note the navbar element exists here!

Screenshot 2022-04-21 at 10 20 20

@gliff-ai gliff-ai deleted a comment from philipjackson Apr 21, 2022
@ChasNelson1990
Copy link
Member

I get the following missing image/icon when I log in?

Also seems true for the other icons

@joshuajames-smith
Copy link
Contributor

I get the following missing image/icon when I log in?

Also seems true for the other icons

Screenshot 2022-06-15 at 13 27 06 Screenshot 2022-06-15 at 13 27 22 Screenshot 2022-06-15 at 13 28 05 Screenshot 2022-06-15 at 13 28 28

I'm also getting this error.

cooper667
cooper667 previously approved these changes Jun 20, 2022
@cooper667 cooper667 requested a review from ChasNelson1990 June 20, 2022 12:38
Copy link
Contributor

@joshuajames-smith joshuajames-smith left a comment

Choose a reason for hiding this comment

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

It's looking good and the issue with the icons is fixed. I got a couple changes for this PR:

  • In AUDIT there is just the team name. Can we use project name and then project name > image name for each of the two AUDIT views? i.e. use CURATE and ANNOTATE content.

@ChasNelson1990
Copy link
Member

@ChasNelson1990
Copy link
Member

@Kinukeo wanna solve the conflicts before I review? Otherwise I'll just have to review again?

@Kinukeo Kinukeo dismissed ChasNelson1990’s stale review July 5, 2022 09:43

All requested changes have been resolved

@Kinukeo Kinukeo merged commit c2fc019 into staging Jul 5, 2022
@Kinukeo Kinukeo deleted the issue306 branch July 5, 2022 09:51
@ChasNelson1990
Copy link
Member

  • This appears to be the default navbar lol:
    image
    It's what I see when I am logged out but not on the sign in/up pages and also on /manage/projects

Just to note, this was because this staging account was veeery old and did not have a team name. All new team's now get a team name by default so this is not actually an issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement [Improvement] Enhancement request. feature [Improvement] New feature request.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants