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

fix(edithomepage): remove optional tags #1450

Merged
merged 1 commit into from
Aug 30, 2023
Merged

Conversation

seaerchin
Copy link
Contributor

@seaerchin seaerchin commented Aug 24, 2023

Problem

notifications and infopic had optional tags, which should be removed per design

Solution

set isRequired to true even though it isn't really :(

Screenshots

Before

Screenshot 2023-08-30 at 2 09 03 PM

After

Screenshot 2023-08-24 at 1 01 37 PM

Tests

  • open hero section
  • notification should not have optional tag after the form's label
  • add infobar
  • infobar should not have optional tag after the form's label

Sorry, something went wrong.

@seaerchin seaerchin requested a review from a team August 24, 2023 05:02
@seaerchin seaerchin temporarily deployed to staging August 24, 2023 05:13 — with GitHub Actions Inactive
@seaerchin seaerchin temporarily deployed to staging August 24, 2023 05:16 — with GitHub Actions Inactive
Copy link
Contributor

@alexanderleegs alexanderleegs left a comment

Choose a reason for hiding this comment

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

Actually the notification banner shouldn't be compulsory though? Not all sites want to have the banner, could we check in with design again on this

Or does this mean that we don't have the optional text but it's actually still optional haha

src/layouts/components/Homepage/HeroBody.tsx Show resolved Hide resolved
src/layouts/components/Homepage/HeroBody.tsx Show resolved Hide resolved
@@ -46,7 +46,7 @@ export const InfopicBody = ({

return (
<Editable.Section>
<FormControl isInvalid={!!errors.subtitle}>
<FormControl isRequired isInvalid={!!errors.subtitle}>
Copy link
Contributor

Choose a reason for hiding this comment

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

notifications and infobar had optional tags, which should be removed per design

@seaerchin so this will mandate people to key in some value? What happens to existing sites where ppl didnt fill them in?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

nope, this is entirely UI only HAHAHA

@seaerchin
Copy link
Contributor Author

Actually the notification banner shouldn't be compulsory though? Not all sites want to have the banner, could we check in with design again on this

Or does this mean that we don't have the optional text but it's actually still optional haha

the latter - no text, still optional. enforcement comes later

@seaerchin seaerchin requested review from a team, alexanderleegs and harishv7 August 30, 2023 06:10
@QiluXie
Copy link
Contributor

QiluXie commented Aug 30, 2023

Actually the notification banner shouldn't be compulsory though? Not all sites want to have the banner, could we check in with design again on this
Or does this mean that we don't have the optional text but it's actually still optional haha

the latter - no text, still optional. enforcement comes later

Enforcement comes later means it will be changed to compulsory later?
A bit confused here, is it our goal to make notification compulsory eventually?

@seaerchin
Copy link
Contributor Author

Actually the notification banner shouldn't be compulsory though? Not all sites want to have the banner, could we check in with design again on this
Or does this mean that we don't have the optional text but it's actually still optional haha

the latter - no text, still optional. enforcement comes later

Enforcement comes later means it will be changed to compulsory later? A bit confused here, is it our goal to make notification compulsory eventually?

we have some fields that are supposed to be compulsory, but the existing validation has a bug. this is fixed in the next PR

@seaerchin seaerchin merged commit ae67b0f into develop Aug 30, 2023
@seaerchin seaerchin deleted the chore/fix-optional-tags branch August 30, 2023 08:55
seaerchin added a commit that referenced this pull request Aug 31, 2023

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
* docs(editable): add docs for drag drop stuf (#1445)

Co-authored-by: seaerchin <[email protected]>

* fix(editable): shift out contxt to allow nested dnd (#1448)

Co-authored-by: seaerchin <[email protected]>

* feat: introduce new version of NotFoundPage (#1435)

* Add submarine image for NotFoundPage

* Add new version of the NotFoundPage

* Add storybook stories for new NotFoundPage

* Fix unit test for RouteSelector

* fix(ui): changes based on design feedback

* IS-322 Setup GrowthBook for FE (#1449)

* feat: add growthbook as dep

* feat: add growthbook to app

* feat: inject user attributes

* feat: add default dev mode to be false

* feat: inject site name as attribute

* feat: add types and featur flag list

* chore: minor fixes

* feat: add type and conditional siteName attribute

* fix: enable dev mode only for local dev

* chore: remove duplicate comment

* IS-417 - Fixing Selector Issues in E2E tests (#1453)

* feat: update text assertion

* fix: resources text assertion

* fix: method to remove other collaborators

* fix: uncomment code

* feat: extract constant

* feat: swap removeFirstCollaborator with removeOtherCollaborators

* fix: use removeOtherCollaborators

* chore: remove unused import and function

* chore(edithomepage): shift constants out (#1416)

* refactor(edithomepage): use new constants

* refactor(edithomepage): shift to new folder

* refactor(edithomepage): shift preview out into own section

---------

Co-authored-by: seaerchin <[email protected]>

* IS-508: Introduce Interface for GrowthBook (#1455)

* feat: introduce interface, remove constants file

* fix: comment out sample flags

* ref(contactus): stylings for contact us page editor (#1458)

* Enhance AddSectionButton to allow adjusting button text

* Add various sections of contact us page

* Adjust Editable to allow new droppable zones

* Update validators to match Figma messages

* Introduce feature-flagged contact us page

* Remove unneeded comment

* Switch to use editable contexts instead and use modal

* Fix validation condition

* Fix name for FeatureFlaggedContactUs

* Remove some unnecessary casts

* Fix RouteSelector unit test

* chore: set pointerEvents to none for draggable icon button (#1456)

* Feat/is 443 nav bar styling (#1457)

* chore: update editable types to allow nav

* feat: add pageMenuBody

* feat: add resourceMenuBody

* feat: add folderMenuBody

* feat: add GroupMenuBody

* feat: swap new components into editNavBar

* chore: fix text for components

* Fix: addSectionButton button text prop

* chore: pass props to button instead of box

* chore: add feature flag for new nav bar (#1459)

* chore: add feature flag for new nav bar

* fix: tests

* feat(feature-flag): add feature flag from gb (#1462)

* feat(feature-flag): add feature flag from gb

* refactor(navbar): use gb for ff

* refactor(feature-flag): add types

---------

Co-authored-by: seaerchin <[email protected]>

* fix(edithomepage): remove optional tags (#1450)

Co-authored-by: seaerchin <[email protected]>

* Feat/is 494 smooth scroll (#1439)

* refactor(edithomepage): smooth scroll

* fix: only scroll after first load

* chore: fix rebase errors

* Revert "fix: only scroll after first load"

This reverts commit 158a4df.

* Revert "refactor(edithomepage): smooth scroll"

This reverts commit e555c2f.

* fix: firstLoad condition

* chore: move first load check to separate method

* fix: smooth scroll

---------

Co-authored-by: seaerchin <[email protected]>
Co-authored-by: Alexander Lee <[email protected]>

* Fix/homepage images on private repos (#1461)

* chore: add util method to retrieve image path

* fix: replace image retrieval in homepage template components with updated v2 endpoint

* fix: change return of util method to handle nested

* Update src/utils/images.ts

Co-authored-by: Qilu Xie <[email protected]>

* Update src/utils/images.ts

Co-authored-by: Qilu Xie <[email protected]>

* chore: fix styling

* refactor: move image fetching to separate hook

---------

Co-authored-by: Qilu Xie <[email protected]>

* 0.41.0

---------

Co-authored-by: seaerchin <[email protected]>
Co-authored-by: seaerchin <[email protected]>
Co-authored-by: Harish <[email protected]>
Co-authored-by: Hsu Zhong Jun <[email protected]>
Co-authored-by: Alexander Lee <[email protected]>
Co-authored-by: Qilu Xie <[email protected]>
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.

None yet

4 participants